From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH vkd3d v2 07/10] vkd3d-shader/hlsl: Support complex numeric initializers.
Message-Id: <f7c2c2d6-6b8f-f996-af06-bd473dfbbbeb@codeweavers.com>
Date: Thu, 13 Jan 2022 12:57:23 -0600
In-Reply-To: <20220110193318.267854-7-fcasas@codeweavers.com>
References: <20220110193318.267854-7-fcasas@codeweavers.com>

On 1/10/22 13:33, Francisco Casas wrote:
> Signed-off-by: Francisco Casas <fcasas@codeweavers.com>
> ---
>   Makefile.am              |  1 -
>   libs/vkd3d-shader/hlsl.y | 72 ++++++++++++++++++++++++++++++++++------
>   2 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index d15e50c3..1086d028 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -298,7 +298,6 @@ XFAIL_TESTS = \
>   	tests/hlsl-initializer-flattening.shader_test \
>   	tests/hlsl-initializer-invalid-n-args.shader_test \
>   	tests/hlsl-initializer-local-array.shader_test \
> -	tests/hlsl-initializer-numeric.shader_test \
>   	tests/hlsl-initializer-static-array.shader_test \
>   	tests/hlsl-initializer-struct.shader_test \
>   	tests/hlsl-bool-cast.shader_test \
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 988e0743..a89e4432 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -1278,6 +1278,54 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem
>       return true;
>   }
>   
> +static void numeric_var_initializer(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
> +        struct parse_variable_def *v, unsigned int reg_offset, struct hlsl_type *type,
> +        unsigned int *initializer_i, struct list *instrs)

I know this follows the naming convention of struct_var_initializer(), 
but as long as we're introducing new code, I'd rather give the function 
a name that has a verb in it.

> +{
> +    unsigned int writemask_offset = 0;
> +    unsigned int components_read = 0;
> +
> +    if (type->type == HLSL_CLASS_MATRIX)
> +        hlsl_fixme(ctx, &var->loc, "Matrix initializer\n");

hlsl_fixme and hlsl_error messages should omit the trailing newline, and 
also be punctuated.

> +
> +    while (components_read < hlsl_type_component_count(type))
> +    {
> +        struct hlsl_ir_store *store;
> +        struct hlsl_ir_constant *c;
> +        struct hlsl_ir_node *node;
> +        unsigned int width;
> +
> +        assert(*initializer_i < v->initializer.args_count);
> +        node = v->initializer.args[*initializer_i];
> +        *initializer_i += 1;

"initializer_i" makes it hard for me to understand what this variable is 
doing. How about "initializer_offset"?

Better yet, what I'd propose is that instead of passing the whole 
parse_variable_def or even parse_initializer, just pass 
"&v->initializer[initializer_offset]" to this function.

> +
> +        width = hlsl_type_component_count(node->data_type);
> +        components_read += width;
> +
> +        if (components_read > hlsl_type_component_count(type))
> +        {
> +            hlsl_error(ctx, &node->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> +                    "Initializer argument has spare components.\n");
> +            return;
> +        }

Isn't this already checked in declare_vars()?

> +
> +        if (!(node = add_implicit_conversion(ctx, instrs, node,
> +                hlsl_get_vector_type(ctx, type->base_type, width), &node->loc)))
> +            return;
> +
> +        if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> +            return;
> +        list_add_tail(instrs, &c->node.entry);
> +
> +        if (!(store = hlsl_new_store(ctx, var, &c->node, node,
> +                ((1u << width) - 1) << writemask_offset, node->loc)))
> +            return;
> +        list_add_tail(instrs, &store->node.entry);
> +
> +        writemask_offset += width;
> +    }
> +}
> +

It's not quite a problem in this patch, since it doesn't implement 
recursion yet, but this won't work with the following shader:

---

uniform float4 u;

float4 main() : sv_target
{
     float4 x[2] = {1, 2, u, 3, 4};
     return x[1];
}

---

Note that "float4 u" can also be e.g. an array or struct, and native 
will still happily allow it.

This suggests to me that what we should probably do is actually 
"flatten" the initializer beforehand. That is, generate load and swizzle 
instructions such that every initializer argument is a scalar. E.g. the 
above would become {1, 2, u.x, u.y, u.z, u.w, 3, 4}. This generates a 
lot of unnecessary scalar ops, but I think we want a vectorization pass 
anyway.

Note that the same applies to constructors. E.g. the following is invalid:

     uniform struct {float2 x, y;} u;
     float4 f = u;

but the following is valid:

     uniform struct {float2 x, y;} u;
     float4 f = float4(u);

I think we should add a helper to "flatten" a parse_initializer, and 
then use it both in constructors and in complex initializers.

One of the nice effects of this is that initializer_size() goes away, 
and a lot of the size checking introduced by this patch series does too.