[PATCH vkd3d 3/6] vkd3d-shader/hlsl: Support complex numeric initializers.

[Francisco Casas]

Signed-off-by: Francisco Casas 
---
 Makefile.am              |  1 -
 libs/vkd3d-shader/hlsl.y | 93 +++++++++++++++++++++++++++++++---------
 2 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 491537f6..85611daf 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -310,7 +310,6 @@ XFAIL_TESTS = \
    tests/hlsl-initializer-invalid-arg-count.shader_test \
    tests/hlsl-initializer-local-array.shader_test \
    tests/hlsl-initializer-nested.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 4ec36b76..bcc9e4d7 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1321,6 +1321,51 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem
     return true;
 }

+static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
+        struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type,
+        unsigned int *initializer_offset)
+{
+    unsigned int i;
+
+    if (type->type == HLSL_CLASS_MATRIX)
+        hlsl_fixme(ctx, &var->loc, "Matrix initializer.");
+
+    for (i = 0; i < type->dimx; i++)
+    {
+        struct hlsl_ir_store *store;
+        struct hlsl_ir_constant *c;
+        struct hlsl_ir_node *node;
+
+        node = initializer->args[*initializer_offset];
+        *initializer_offset += 1;
+
+        if (!(node = add_implicit_conversion(ctx, initializer->instrs, node,
+                hlsl_get_scalar_type(ctx, type->base_type), &node->loc)))
+            return;
+
+        if (hlsl_type_is_single_reg(var->data_type))
+        {
+            if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
+                return;
+            list_add_tail(initializer->instrs, &c->node.entry);
+
+            if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc)))
+                return;
+        }
+        else
+        {
+            if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc)))
+                return;
+            list_add_tail(initializer->instrs, &c->node.entry);
+
+            if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
+                return;
+        }
+
+        list_add_tail(initializer->instrs, &store->node.entry);
+    }
+}
+
 static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var,
         struct parse_initializer *initializer)
 {
@@ -1492,20 +1537,16 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
         if (v->initializer.args_count)
         {
             unsigned int size = initializer_size(&v->initializer);
-            struct hlsl_ir_load *load;

-            if (type->type <= HLSL_CLASS_LAST_NUMERIC
-                    && type->dimx * type->dimy != size && size != 1)
+            if (type->type <= HLSL_CLASS_LAST_NUMERIC && v->initializer.args_count != 1
+                    && type->dimx * type->dimy != size)
             {
-                if (size < type->dimx * type->dimy)
-                {
-                    hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
-                            "Expected %u components in numeric initializer, but got %u.",
-                            type->dimx * type->dimy, size);
-                    free_parse_initializer(&v->initializer);
-                    vkd3d_free(v);
-                    continue;
-                }
+                hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
+                        "Expected %u components in numeric initializer, but got %u.",
+                        type->dimx * type->dimy, v->initializer.args_count);
+                free_parse_initializer(&v->initializer);
+                vkd3d_free(v);
+                continue;
             }
             if ((type->type == HLSL_CLASS_STRUCT || type->type == HLSL_CLASS_ARRAY)
                     && hlsl_type_component_count(type) != size)
@@ -1540,23 +1581,35 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
                 vkd3d_free(v);
                 continue;
             }
+
             if (v->initializer.args_count > 1)
             {
-                hlsl_fixme(ctx, &v->loc, "Complex initializer.");
-                free_parse_initializer(&v->initializer);
-                vkd3d_free(v);
-                continue;
+                unsigned int initializer_offset = 0;
+
+                if (v->initializer.args_count != size)
+                {
+                    hlsl_fixme(ctx, &v->loc, "Flatten initializer.");
+                    free_parse_initializer(&v->initializer);
+                    vkd3d_free(v);
+                    continue;
+                }
+
+                initialize_numeric_var(ctx, var, &v->initializer, 0, type, &initializer_offset);
             }
+            else
+            {
+                struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc);

-            load = hlsl_new_var_load(ctx, var, var->loc);
-            list_add_tail(v->initializer.instrs, &load->node.entry);
-            add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
-            vkd3d_free(v->initializer.args);
+                list_add_tail(v->initializer.instrs, &load->node.entry);
+                add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
+            }

if (modifiers & HLSL_STORAGE_STATIC)
                 list_move_tail(&ctx->static_initializers, v->initializer.instrs);
             else
                 list_move_tail(statements_list, v->initializer.instrs);
+
+            vkd3d_free(v->initializer.args);
             vkd3d_free(v->initializer.instrs);
         }

[Giovanni Mascellani]

Hi,

the patch looks mostly ok, with a few comments.

Il 15/02/22 21:17, Francisco Casas ha scritto:
> +static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
> -
>    struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type,
> -
>    unsigned int *initializer_offset)
>
> +{

Isn't "type" here the same thing as "var->data_type" in the only call to 
this function? If so, you could avoid passing "type" explicitly.
> - unsigned int i;
> -
> - if (type->type == HLSL_CLASS_MATRIX)
> -
>    hlsl_fixme(ctx, &var->loc, "Matrix initializer.");
> -
> - for (i = 0; i < type->dimx; i++)
> - {
> -
>    struct hlsl_ir_store *store;
> -
>    struct hlsl_ir_constant *c;
> -
>    struct hlsl_ir_node *node;
> -
> -
>    node = initializer->args[*initializer_offset];
> -
>    *initializer_offset += 1;
> -
> -
>    if (!(node = add_implicit_conversion(ctx, initializer->instrs, node,
> -
>            hlsl_get_scalar_type(ctx, type->base_type), &node->loc)))
> -
>        return;
> -
> -
>    if (hlsl_type_is_single_reg(var->data_type))
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc)))
> -
>            return;
> -
>    }
> -
>    else
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
> -
>            return;
> -
>    }

Again under the assumption that "type == var->data_type", here it seems 
that you're handling the matrix case, while it is marked as fixme above. 
I guess the reason for the fixme is that you're only iterating on dimx? 
Still, I find it a bit confusing. If you consider the matrix case not 
(yet) supported, I'd just avoid to add pieces of it until you're really 
supporting it. Though maybe I missing something.

Thanks, Giovanni.

[Francisco Casas]

Hello

February 16, 2022 2:14 PM, "Giovanni Mascellani"  wrote:
> Hi,
>
> the patch looks mostly ok, with a few comments.
>
> Il 15/02/22 21:17, Francisco Casas ha scritto:
> > +static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
> > - struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type,
> > - unsigned int *initializer_offset)
> >
> > +{
>
> Isn't "type" here the same thing as "var->data_type" in the only call to this function? If so, you
> could avoid passing "type" explicitly.

For now it is the same thing but 4 patches ahead (in the continuation of this batch) that will no
longer be true.
The idea is that var may also be a struct or an array, and type would be the type of the numeric 
field inside that we want to initialize.
> > - unsigned int i;
> > -
> > - if (type->type == HLSL_CLASS_MATRIX)
> > - hlsl_fixme(ctx, &var->loc, "Matrix initializer.");
> > -
> > - for (i = 0; i < type->dimx; i++)
> > - {
> > - struct hlsl_ir_store *store;
> > - struct hlsl_ir_constant *c;
> > - struct hlsl_ir_node *node;
> > -
> > - node = initializer->args[*initializer_offset];
> > - *initializer_offset += 1;
> > -
> > - if (!(node = add_implicit_conversion(ctx, initializer->instrs, node,
> > - hlsl_get_scalar_type(ctx, type->base_type), &node->loc)))
> > - return;
> > -
> > - if (hlsl_type_is_single_reg(var->data_type))
> > - {
> > - if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> > - return;
> > - list_add_tail(initializer->instrs, &c->node.entry);
> > -
> > - if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc)))
> > - return;
> > - }
> > - else
> > - {
> > - if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc)))
> > - return;
> > - list_add_tail(initializer->instrs, &c->node.entry);
> > -
> > - if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
> > - return;
> > - }
>
> Again under the assumption that "type == var->data_type", here it seems that you're handling the
> matrix case, while it is marked as fixme above. I guess the reason for the fixme is that you're
> only iterating on dimx? Still, I find it a bit confusing. If you consider the matrix case not (yet)
> supported, I'd just avoid to add pieces of it until you're really supporting it. Though maybe I
> missing something.

As before, "type == var->data_type" won't hold after

33e27ff7 vkd3d-shader/hlsl: Use generic variable initializer for struct fields.

which would have been the 7th patch of this batch if I hadn't split it.
So it is not just for the matrix case.
(I implemented matrix initializers too but they are 10 initializer patches ahead.)

Best regards,
Francisco

[Giovanni Mascellani]

Hi,

Il 16/02/22 18:59, Francisco Casas ha scritto:
> For now it is the same thing but 4 patches ahead (in the continuation of this batch) that will no
> longer be true.
> The idea is that var may also be a struct or an array, and type would be the type of the numeric
> field inside that we want to initialize.

Yeah, it makes sense.

I think the patches in this series are correct, meaning that as far as I 
can see they do not introduce errors and the general structure of the 
code looks fine. Clearly they leave the code in a very "in the making" 
state, but that's reasonable considering that you still have other work 
to submit and are working actively on this thing, so I am signing off on 
them anyway.

Once you are done with all your patches I might still have a few 
comments, but they are minor enough that to me it makes sense to first 
let the patches in and then, at the end, fix the small leftovers.

Giovanni.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani

Il 15/02/22 21:17, Francisco Casas ha scritto:
> Signed-off-by: Francisco Casas
>
> Makefile.am              |  1 -
>   libs/vkd3d-shader/hlsl.y | 93 +++++++++++++++++++++++++++++++---------
>   2 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 491537f6..85611daf 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -310,7 +310,6 @@ XFAIL_TESTS = \
>    tests/hlsl-initializer-invalid-arg-count.shader_test \
>    tests/hlsl-initializer-local-array.shader_test \
>    tests/hlsl-initializer-nested.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 4ec36b76..bcc9e4d7 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -1321,6 +1321,51 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem
>       return true;
>   }
>
> +static void initialize_numeric_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
> -
>    struct parse_initializer *initializer, unsigned int reg_offset, struct hlsl_type *type,
> -
>    unsigned int *initializer_offset)
>
> +{
> - unsigned int i;
> -
> - if (type->type == HLSL_CLASS_MATRIX)
> -
>    hlsl_fixme(ctx, &var->loc, "Matrix initializer.");
> -
> - for (i = 0; i < type->dimx; i++)
> - {
> -
>    struct hlsl_ir_store *store;
> -
>    struct hlsl_ir_constant *c;
> -
>    struct hlsl_ir_node *node;
> -
> -
>    node = initializer->args[*initializer_offset];
> -
>    *initializer_offset += 1;
> -
> -
>    if (!(node = add_implicit_conversion(ctx, initializer->instrs, node,
> -
>            hlsl_get_scalar_type(ctx, type->base_type), &node->loc)))
> -
>        return;
> -
> -
>    if (hlsl_type_is_single_reg(var->data_type))
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, (1u << i), node->loc)))
> -
>            return;
> -
>    }
> -
>    else
> -
>    {
> -
>        if (!(c = hlsl_new_uint_constant(ctx, reg_offset + i, node->loc)))
> -
>            return;
> -
>        list_add_tail(initializer->instrs, &c->node.entry);
> -
> -
>        if (!(store = hlsl_new_store(ctx, var, &c->node, node, 0, node->loc)))
> -
>            return;
> -
>    }
> -
> -
>    list_add_tail(initializer->instrs, &store->node.entry);
> - }
>
> +}
> - static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var,
>         struct parse_initializer *initializer)
> {
>
> @@ -1492,20 +1537,16 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
>           if (v->initializer.args_count)
>           {
>               unsigned int size = initializer_size(&v->initializer);
> -
>        struct hlsl_ir_load *load;
>
> -
>        if (type->type <= HLSL_CLASS_LAST_NUMERIC
> -
>                && type->dimx * type->dimy != size && size != 1)
> -
>        if (type->type <= HLSL_CLASS_LAST_NUMERIC && v->initializer.args_count != 1
> -
>                && type->dimx * type->dimy != size)
>         {
> -
>            if (size < type->dimx * type->dimy)
> -
>            {
> -
>                hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> -
>                        "Expected %u components in numeric initializer, but got %u.",
> -
>                        type->dimx * type->dimy, size);
> -
>                free_parse_initializer(&v->initializer);
> -
>                vkd3d_free(v);
> -
>                continue;
> -
>            }
> -
>            hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> -
>                    "Expected %u components in numeric initializer, but got %u.",
> -
>                    type->dimx * type->dimy, v->initializer.args_count);
> -
>            free_parse_initializer(&v->initializer);
> -
>            vkd3d_free(v);
> -
>            continue;
>         }
>         if ((type->type == HLSL_CLASS_STRUCT || type->type == HLSL_CLASS_ARRAY)
>                 && hlsl_type_component_count(type) != size)
>
> @@ -1540,23 +1581,35 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
>                   vkd3d_free(v);
>                   continue;
>               }
> -
>         if (v->initializer.args_count > 1)
>         {
> -
>            hlsl_fixme(ctx, &v->loc, "Complex initializer.");
> -
>            free_parse_initializer(&v->initializer);
> -
>            vkd3d_free(v);
> -
>            continue;
> -
>            unsigned int initializer_offset = 0;
> -
> -
>            if (v->initializer.args_count != size)
> -
>            {
> -
>                hlsl_fixme(ctx, &v->loc, "Flatten initializer.");
> -
>                free_parse_initializer(&v->initializer);
> -
>                vkd3d_free(v);
> -
>                continue;
> -
>            }
> -
> -
>            initialize_numeric_var(ctx, var, &v->initializer, 0, type, &initializer_offset);
>         }
> -
>        else
> -
>        {
> -
>            struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc);
>
> -
>        load = hlsl_new_var_load(ctx, var, var->loc);
> -
>        list_add_tail(v->initializer.instrs, &load->node.entry);
> -
>        add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
> -
>        vkd3d_free(v->initializer.args);
> -
>            list_add_tail(v->initializer.instrs, &load->node.entry);
> -
>            add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
> -
>        }
>
>         if (modifiers & HLSL_STORAGE_STATIC)
>             list_move_tail(&ctx->static_initializers, v->initializer.instrs);
>         else
>             list_move_tail(statements_list, v->initializer.instrs);
> -
> -
>        vkd3d_free(v->initializer.args);
>         vkd3d_free(v->initializer.instrs);
>     }