From: Zebediah Figura <zfigura@codeweavers.com>
Subject: Re: [PATCH vkd3d 1/5] vkd3d-shader/hlsl: Write SM4 casts from bool.
Message-Id: <f0fa581f-5738-3553-70aa-458356b2f0a9@codeweavers.com>
Date: Thu, 30 Jun 2022 23:43:19 -0500
In-Reply-To: <20220629145555.871548-1-gmascellani@codeweavers.com>
References: <20220629145555.871548-1-gmascellani@codeweavers.com>

On 6/29/22 09:55, Giovanni Mascellani wrote:
> Signed-off-by: Giovanni Mascellani <gmascellani@codeweavers.com>
> ---
>   libs/vkd3d-shader/hlsl_sm4.c       | 44 ++++++++++++++++++++++++++++--
>   tests/cast-to-float.shader_test    |  2 +-
>   tests/cast-to-int.shader_test      |  4 +--
>   tests/cast-to-uint.shader_test     |  4 +--
>   tests/hlsl-bool-cast.shader_test   | 18 ++++++++++++
>   tests/logic-operations.shader_test |  6 ++--
>   6 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
> index 79027169..23678498 100644
> --- a/libs/vkd3d-shader/hlsl_sm4.c
> +++ b/libs/vkd3d-shader/hlsl_sm4.c
> @@ -1459,6 +1459,44 @@ static bool type_is_float(const struct hlsl_type *type)
>       return type->base_type == HLSL_TYPE_FLOAT || type->base_type == HLSL_TYPE_HALF;
>   }
>   
> +static void write_sm4_cast_from_bool(struct hlsl_ctx *ctx,
> +        struct vkd3d_bytecode_buffer *buffer, const struct hlsl_ir_expr *expr,
> +        const struct hlsl_ir_node *arg)
> +{
> +    struct sm4_instruction instr;
> +    uint32_t c;
> +
> +    switch (expr->node.data_type->base_type)
> +    {
> +        case HLSL_TYPE_FLOAT:

Should this include HLSL_TYPE_HALF as well?

> +            c = 0x3f800000;
> +            break;

Maybe not worth worrying about, but the hardcoded number could be 
avoided by doing something like

union
{
      uint32_t u;
      float f;
} c;

...

c.f = 1.0f;

> +
> +        case HLSL_TYPE_INT:
> +        case HLSL_TYPE_UINT:
> +            c = 1;
> +            break;
> +
> +        default:
> +            assert(0);

We don't need to handle doubles, but I'm not sure they should result in 
an assert either.

> +    }
> +
> +    memset(&instr, 0, sizeof(instr));
> +    instr.opcode = VKD3D_SM4_OP_AND;
> +
> +    sm4_dst_from_node(&instr.dsts[0], &expr->node);
> +    instr.dst_count = 1;
> +
> +    sm4_src_from_node(&instr.srcs[0], arg, instr.dsts[0].writemask);
> +    instr.srcs[1].swizzle_type = VKD3D_SM4_SWIZZLE_NONE;
> +    instr.srcs[1].reg.type = VKD3D_SM4_RT_IMMCONST;
> +    instr.srcs[1].reg.dim = VKD3D_SM4_DIMENSION_SCALAR;
> +    instr.srcs[1].reg.immconst_uint[0] = c;
> +    instr.src_count = 2;
> +
> +    write_sm4_instruction(buffer, &instr);
> +}

I kind of wonder if this should be a lowering pass instead. It wouldn't 
apply to sm1, but it would (maybe?) be a bit easier to write, and it 
would apply to a hypothetical sm6 backend, if we indeed write our own 
instead of leveraging the Microsoft compiler.

Fine to leave it as is for now, I think, but food for thought.

> +
>   static void write_sm4_cast(struct hlsl_ctx *ctx,
>           struct vkd3d_bytecode_buffer *buffer, const struct hlsl_ir_expr *expr)
>   {
> @@ -1488,7 +1526,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx,
>                       break;
>   
>                   case HLSL_TYPE_BOOL:
> -                    hlsl_fixme(ctx, &expr->node.loc, "SM4 cast from bool to float.");
> +                    write_sm4_cast_from_bool(ctx, buffer, expr, arg1);
>                       break;
>   
>                   case HLSL_TYPE_DOUBLE:
> @@ -1514,7 +1552,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx,
>                       break;
>   
>                   case HLSL_TYPE_BOOL:
> -                    hlsl_fixme(ctx, &expr->node.loc, "SM4 cast from bool to int.");
> +                    write_sm4_cast_from_bool(ctx, buffer, expr, arg1);
>                       break;
>   
>                   case HLSL_TYPE_DOUBLE:
> @@ -1540,7 +1578,7 @@ static void write_sm4_cast(struct hlsl_ctx *ctx,
>                       break;
>   
>                   case HLSL_TYPE_BOOL:
> -                    hlsl_fixme(ctx, &expr->node.loc, "SM4 cast from bool to uint.");
> +                    write_sm4_cast_from_bool(ctx, buffer, expr, arg1);
>                       break;
>   
>                   case HLSL_TYPE_DOUBLE:

Could we just pass the mask from here instead, and skip the switch 
inside of write_sm4_cast_from_bool()?