[PATCH 1/5] d3dcompiler: Synthesize a variable when subscripting a non-deref node.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
There are of course other options to handle this situation, e.g. allowing
hlsl_deref to contain either hlsl_ir_var or hlsl_ir_node, or introducing another
deref type (e.g. HLSL_IR_DEREF and HLSL_IR_OFFSET).

I'm inclined to go with this one, mostly because we don't need to handle
multiple cases when dealing with derefs (or another case when dealing with node
types).

 dlls/d3dcompiler_43/hlsl.y | 68 +++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index db305361738..01b210419c8 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -125,6 +125,11 @@ static void check_invalid_matrix_modifiers(DWORD modifiers, struct source_locati
     }
 }
 
+static BOOL type_is_single_reg(const struct hlsl_type *type)
+{
+    return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
+}
+
 static BOOL declare_variable(struct hlsl_ir_var *decl, BOOL local)
 {
     BOOL ret;
@@ -507,6 +512,41 @@ static struct hlsl_ir_jump *new_return(struct hlsl_ir_node *value, struct source
     return jump;
 }
 
+static struct hlsl_ir_var *new_synthetic_var(const char *name, struct hlsl_type *type,
+        const struct source_location loc)
+{
+    struct hlsl_ir_var *var;
+
+    if (!(var = d3dcompiler_alloc(sizeof(*var))))
+    {
+        hlsl_ctx.status = PARSE_ERR;
+        return NULL;
+    }
+
+    var->name = strdup(name);
+    var->data_type = type;
+    var->loc = loc;
+    list_add_tail(&hlsl_ctx.globals->vars, &var->scope_entry);
+    return var;
+}
+
+static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs)
+{
+    struct hlsl_ir_assignment *assign;
+
+    if (!(assign = d3dcompiler_alloc(sizeof(*assign))))
+        return NULL;
+
+    init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
+    assign->lhs.type = HLSL_IR_DEREF_VAR;
+    assign->lhs.v.var = lhs;
+    assign->rhs = rhs;
+    if (type_is_single_reg(lhs->data_type))
+        assign->writemask = (1 << lhs->data_type->dimx) - 1;
+
+    return assign;
+}
+
 static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
 {
     struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
@@ -525,13 +565,33 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
 static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
         struct hlsl_struct_field *field, const struct source_location loc)
 {
-    struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
+    struct hlsl_ir_deref *deref;
 
-    if (!deref)
+    if (record->type != HLSL_IR_DEREF)
     {
-        ERR("Out of memory.\n");
-        return NULL;
+        struct hlsl_ir_assignment *assign;
+        struct hlsl_ir_var *var;
+        char name[27];
+
+        sprintf(name, "<deref-%p>", record);
+        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
+            return NULL;
+
+        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
+
+        if (!(assign = make_simple_assignment(var, record)))
+            return NULL;
+        list_add_after(&record->entry, &assign->node.entry);
+
+        if (!(deref = new_var_deref(var, var->loc)))
+            return NULL;
+        list_add_after(&assign->node.entry, &deref->node.entry);
+        record = &deref->node;
     }
+
+    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
+        return NULL;
+
     init_node(&deref->node, HLSL_IR_DEREF, field->type, loc);
     deref->src.type = HLSL_IR_DEREF_RECORD;
     deref->src.v.record.record = record;
-- 
2.26.2

[PATCH 2/5] d3dcompiler: Synthesize a variable when indexing a non-deref node.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/hlsl.y | 122 +++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 52 deletions(-)

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 01b210419c8..5875705cf18 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -562,32 +562,39 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
     return deref;
 }
 
-static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
-        struct hlsl_struct_field *field, const struct source_location loc)
+static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
 {
+    struct hlsl_ir_assignment *assign;
     struct hlsl_ir_deref *deref;
+    struct hlsl_ir_var *var;
+    char name[27];
 
-    if (record->type != HLSL_IR_DEREF)
-    {
-        struct hlsl_ir_assignment *assign;
-        struct hlsl_ir_var *var;
-        char name[27];
+    if (node->type == HLSL_IR_DEREF)
+        return node;
 
-        sprintf(name, "<deref-%p>", record);
-        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
-            return NULL;
+    sprintf(name, "<deref-%p>", node);
+    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
+        return NULL;
 
-        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
+    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
 
-        if (!(assign = make_simple_assignment(var, record)))
-            return NULL;
-        list_add_after(&record->entry, &assign->node.entry);
+    if (!(assign = make_simple_assignment(var, node)))
+        return NULL;
+    list_add_after(&node->entry, &assign->node.entry);
 
-        if (!(deref = new_var_deref(var, var->loc)))
-            return NULL;
-        list_add_after(&assign->node.entry, &deref->node.entry);
-        record = &deref->node;
-    }
+    if (!(deref = new_var_deref(var, var->loc)))
+        return NULL;
+    list_add_after(&assign->node.entry, &deref->node.entry);
+    return &deref->node;
+}
+
+static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
+        struct hlsl_struct_field *field, const struct source_location loc)
+{
+    struct hlsl_ir_deref *deref;
+
+    if (!(record = get_var_deref(record)))
+        return NULL;
 
     if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
         return NULL;
@@ -599,6 +606,49 @@ static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
     return deref;
 }
 
+static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
+        struct hlsl_ir_node *index, const struct source_location loc)
+{
+    const struct hlsl_type *expr_type = array->data_type;
+    struct hlsl_ir_deref *deref;
+    struct hlsl_type *data_type;
+
+    TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
+
+    if (expr_type->type == HLSL_CLASS_ARRAY)
+    {
+        data_type = expr_type->e.array.type;
+    }
+    else if (expr_type->type == HLSL_CLASS_MATRIX)
+    {
+        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
+    }
+    else if (expr_type->type == HLSL_CLASS_VECTOR)
+    {
+        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
+    }
+    else
+    {
+        if (expr_type->type == HLSL_CLASS_SCALAR)
+            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "array-indexed expression is scalar");
+        else
+            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "expression is not array-indexable");
+        return NULL;
+    }
+
+    if (!(array = get_var_deref(array)))
+        return NULL;
+
+    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
+        return NULL;
+
+    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
+    deref->src.type = HLSL_IR_DEREF_ARRAY;
+    deref->src.v.array.array = array;
+    deref->src.v.array.index = index;
+    return deref;
+}
+
 static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
         struct parse_initializer *initializer)
 {
@@ -2290,34 +2340,7 @@ postfix_expr:             primary_expr
                                 /* This may be an array dereference or a vector/matrix
                                  * subcomponent access.
                                  * We store it as an array dereference in any case. */
-                                const struct hlsl_type *expr_type = node_from_list($1)->data_type;
                                 struct hlsl_ir_deref *deref;
-                                struct hlsl_type *data_type;
-
-                                TRACE("Array dereference from type %s\n", debug_hlsl_type(expr_type));
-
-                                if (expr_type->type == HLSL_CLASS_ARRAY)
-                                {
-                                    data_type = expr_type->e.array.type;
-                                }
-                                else if (expr_type->type == HLSL_CLASS_MATRIX)
-                                {
-                                    data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
-                                }
-                                else if (expr_type->type == HLSL_CLASS_VECTOR)
-                                {
-                                    data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
-                                }
-                                else
-                                {
-                                    if (expr_type->type == HLSL_CLASS_SCALAR)
-                                        hlsl_report_message(get_location(&@2), HLSL_LEVEL_ERROR, "array-indexed expression is scalar");
-                                    else
-                                        hlsl_report_message(get_location(&@2), HLSL_LEVEL_ERROR, "expression is not array-indexable");
-                                    free_instr_list($1);
-                                    free_instr_list($3);
-                                    YYABORT;
-                                }
 
                                 if (node_from_list($3)->data_type->type != HLSL_CLASS_SCALAR)
                                 {
@@ -2327,17 +2350,12 @@ postfix_expr:             primary_expr
                                     YYABORT;
                                 }
 
-                                if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
+                                if (!(deref = new_array_deref(node_from_list($1), node_from_list($3), get_location(&@2))))
                                 {
                                     free_instr_list($1);
                                     free_instr_list($3);
                                     YYABORT;
                                 }
-                                init_node(&deref->node, HLSL_IR_DEREF, data_type, get_location(&@2));
-                                deref->src.type = HLSL_IR_DEREF_ARRAY;
-                                deref->src.v.array.array = node_from_list($1);
-                                deref->src.v.array.index = node_from_list($3);
-
                                 $$ = append_binop($1, $3, &deref->node);
                             }
                           /* "var_modifiers" doesn't make sense in this case, but it's needed
-- 
2.26.2

[PATCH 2/5] d3dcompiler: Synthesize a variable when indexing a non-deref node.

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>

[PATCH 2/5] d3dcompiler: Synthesize a variable when indexing a non-deref node.

[Matteo Bruni]

On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  dlls/d3dcompiler_43/hlsl.y | 122 +++++++++++++++++++++----------------
>  1 file changed, 70 insertions(+), 52 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index 01b210419c8..5875705cf18 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -562,32 +562,39 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
>      return deref;
>  }
>
> -static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> -        struct hlsl_struct_field *field, const struct source_location loc)
> +static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
>  {
> +    struct hlsl_ir_assignment *assign;
>      struct hlsl_ir_deref *deref;
> +    struct hlsl_ir_var *var;
> +    char name[27];
>
> -    if (record->type != HLSL_IR_DEREF)
> -    {
> -        struct hlsl_ir_assignment *assign;
> -        struct hlsl_ir_var *var;
> -        char name[27];
> +    if (node->type == HLSL_IR_DEREF)
> +        return node;
>
> -        sprintf(name, "<deref-%p>", record);
> -        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
> -            return NULL;
> +    sprintf(name, "<deref-%p>", node);
> +    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> +        return NULL;
>
> -        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
> +    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
>
> -        if (!(assign = make_simple_assignment(var, record)))
> -            return NULL;
> -        list_add_after(&record->entry, &assign->node.entry);
> +    if (!(assign = make_simple_assignment(var, node)))
> +        return NULL;
> +    list_add_after(&node->entry, &assign->node.entry);
>
> -        if (!(deref = new_var_deref(var, var->loc)))
> -            return NULL;
> -        list_add_after(&assign->node.entry, &deref->node.entry);
> -        record = &deref->node;
> -    }
> +    if (!(deref = new_var_deref(var, var->loc)))
> +        return NULL;
> +    list_add_after(&assign->node.entry, &deref->node.entry);
> +    return &deref->node;
> +}
> +
> +static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> +        struct hlsl_struct_field *field, const struct source_location loc)
> +{
> +    struct hlsl_ir_deref *deref;
> +
> +    if (!(record = get_var_deref(record)))
> +        return NULL;
>
> if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
>          return NULL;
> @@ -599,6 +606,49 @@ static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
>      return deref;
>  }
>
> +static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
> +        struct hlsl_ir_node *index, const struct source_location loc)
> +{
> +    const struct hlsl_type *expr_type = array->data_type;
> +    struct hlsl_ir_deref *deref;
> +    struct hlsl_type *data_type;
> +
> +    TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
> +
> +    if (expr_type->type == HLSL_CLASS_ARRAY)
> +    {
> +        data_type = expr_type->e.array.type;
> +    }
> +    else if (expr_type->type == HLSL_CLASS_MATRIX)
> +    {
> +        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> +    }
> +    else if (expr_type->type == HLSL_CLASS_VECTOR)
> +    {
> +        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> +    }
> +    else
> +    {
> +        if (expr_type->type == HLSL_CLASS_SCALAR)
> +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "array-indexed expression is scalar");
> +        else
> +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "expression is not array-indexable");
> +        return NULL;
> +    }
> +
> +    if (!(array = get_var_deref(array)))
> +        return NULL;
> +
> +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> +        return NULL;
> +
> +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> +    deref->src.type = HLSL_IR_DEREF_ARRAY;
> +    deref->src.v.array.array = array;
> +    deref->src.v.array.index = index;
> +    return deref;
> +}

It should be possible to have a sensible test for this one instead.
Care to write one?

[PATCH 2/5] d3dcompiler: Synthesize a variable when indexing a non-deref node.

[Zebediah Figura]

On 5/19/20 2:31 PM, Matteo Bruni wrote:
> On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> >  dlls/d3dcompiler_43/hlsl.y | 122 +++++++++++++++++++++----------------
> >  1 file changed, 70 insertions(+), 52 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index 01b210419c8..5875705cf18 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -562,32 +562,39 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> >      return deref;
> >  }
> >
> > -static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> > -        struct hlsl_struct_field *field, const struct source_location loc)
> > +static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> >  {
> > +    struct hlsl_ir_assignment *assign;
> >      struct hlsl_ir_deref *deref;
> > +    struct hlsl_ir_var *var;
> > +    char name[27];
> >
> > -    if (record->type != HLSL_IR_DEREF)
> > -    {
> > -        struct hlsl_ir_assignment *assign;
> > -        struct hlsl_ir_var *var;
> > -        char name[27];
> > +    if (node->type == HLSL_IR_DEREF)
> > +        return node;
> >
> > -        sprintf(name, "<deref-%p>", record);
> > -        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
> > -            return NULL;
> > +    sprintf(name, "<deref-%p>", node);
> > +    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> > +        return NULL;
> >
> > -        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
> > +    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> >
> > -        if (!(assign = make_simple_assignment(var, record)))
> > -            return NULL;
> > -        list_add_after(&record->entry, &assign->node.entry);
> > +    if (!(assign = make_simple_assignment(var, node)))
> > +        return NULL;
> > +    list_add_after(&node->entry, &assign->node.entry);
> >
> > -        if (!(deref = new_var_deref(var, var->loc)))
> > -            return NULL;
> > -        list_add_after(&assign->node.entry, &deref->node.entry);
> > -        record = &deref->node;
> > -    }
> > +    if (!(deref = new_var_deref(var, var->loc)))
> > +        return NULL;
> > +    list_add_after(&assign->node.entry, &deref->node.entry);
> > +    return &deref->node;
> > +}
> > +
> > +static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> > +        struct hlsl_struct_field *field, const struct source_location loc)
> > +{
> > +    struct hlsl_ir_deref *deref;
> > +
> > +    if (!(record = get_var_deref(record)))
> > +        return NULL;
> >
> > if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> >          return NULL;
> > @@ -599,6 +606,49 @@ static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> >      return deref;
> >  }
> >
> > +static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
> > +        struct hlsl_ir_node *index, const struct source_location loc)
> > +{
> > +    const struct hlsl_type *expr_type = array->data_type;
> > +    struct hlsl_ir_deref *deref;
> > +    struct hlsl_type *data_type;
> > +
> > +    TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
> > +
> > +    if (expr_type->type == HLSL_CLASS_ARRAY)
> > +    {
> > +        data_type = expr_type->e.array.type;
> > +    }
> > +    else if (expr_type->type == HLSL_CLASS_MATRIX)
> > +    {
> > +        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> > +    }
> > +    else if (expr_type->type == HLSL_CLASS_VECTOR)
> > +    {
> > +        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> > +    }
> > +    else
> > +    {
> > +        if (expr_type->type == HLSL_CLASS_SCALAR)
> > +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "array-indexed expression is scalar");
> > +        else
> > +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "expression is not array-indexable");
> > +        return NULL;
> > +    }
> > +
> > +    if (!(array = get_var_deref(array)))
> > +        return NULL;
> > +
> > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > +        return NULL;
> > +
> > +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> > +    deref->src.type = HLSL_IR_DEREF_ARRAY;
> > +    deref->src.v.array.array = array;
> > +    deref->src.v.array.index = index;
> > +    return deref;
> > +}
>
> It should be possible to have a sensible test for this one instead.
> Care to write one?

Inasmuch as expressions like (a + b)[i] are sensible, I guess...

[PATCH 2/5] d3dcompiler: Synthesize a variable when indexing a non-deref node.

[Matteo Bruni]

On Tue, May 19, 2020 at 9:38 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 5/19/20 2:31 PM, Matteo Bruni wrote:
> > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > ---
> > >  dlls/d3dcompiler_43/hlsl.y | 122 +++++++++++++++++++++----------------
> > >  1 file changed, 70 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > index 01b210419c8..5875705cf18 100644
> > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > @@ -562,32 +562,39 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> > >      return deref;
> > >  }
> > >
> > > -static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> > > -        struct hlsl_struct_field *field, const struct source_location loc)
> > > +static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> > >  {
> > > +    struct hlsl_ir_assignment *assign;
> > >      struct hlsl_ir_deref *deref;
> > > +    struct hlsl_ir_var *var;
> > > +    char name[27];
> > >
> > > -    if (record->type != HLSL_IR_DEREF)
> > > -    {
> > > -        struct hlsl_ir_assignment *assign;
> > > -        struct hlsl_ir_var *var;
> > > -        char name[27];
> > > +    if (node->type == HLSL_IR_DEREF)
> > > +        return node;
> > >
> > > -        sprintf(name, "<deref-%p>", record);
> > > -        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
> > > -            return NULL;
> > > +    sprintf(name, "<deref-%p>", node);
> > > +    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> > > +        return NULL;
> > >
> > > -        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
> > > +    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> > >
> > > -        if (!(assign = make_simple_assignment(var, record)))
> > > -            return NULL;
> > > -        list_add_after(&record->entry, &assign->node.entry);
> > > +    if (!(assign = make_simple_assignment(var, node)))
> > > +        return NULL;
> > > +    list_add_after(&node->entry, &assign->node.entry);
> > >
> > > -        if (!(deref = new_var_deref(var, var->loc)))
> > > -            return NULL;
> > > -        list_add_after(&assign->node.entry, &deref->node.entry);
> > > -        record = &deref->node;
> > > -    }
> > > +    if (!(deref = new_var_deref(var, var->loc)))
> > > +        return NULL;
> > > +    list_add_after(&assign->node.entry, &deref->node.entry);
> > > +    return &deref->node;
> > > +}
> > > +
> > > +static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> > > +        struct hlsl_struct_field *field, const struct source_location loc)
> > > +{
> > > +    struct hlsl_ir_deref *deref;
> > > +
> > > +    if (!(record = get_var_deref(record)))
> > > +        return NULL;
> > >
> > > if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > >          return NULL;
> > > @@ -599,6 +606,49 @@ static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> > >      return deref;
> > >  }
> > >
> > > +static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
> > > +        struct hlsl_ir_node *index, const struct source_location loc)
> > > +{
> > > +    const struct hlsl_type *expr_type = array->data_type;
> > > +    struct hlsl_ir_deref *deref;
> > > +    struct hlsl_type *data_type;
> > > +
> > > +    TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
> > > +
> > > +    if (expr_type->type == HLSL_CLASS_ARRAY)
> > > +    {
> > > +        data_type = expr_type->e.array.type;
> > > +    }
> > > +    else if (expr_type->type == HLSL_CLASS_MATRIX)
> > > +    {
> > > +        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> > > +    }
> > > +    else if (expr_type->type == HLSL_CLASS_VECTOR)
> > > +    {
> > > +        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> > > +    }
> > > +    else
> > > +    {
> > > +        if (expr_type->type == HLSL_CLASS_SCALAR)
> > > +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "array-indexed expression is scalar");
> > > +        else
> > > +            hlsl_report_message(loc, HLSL_LEVEL_ERROR, "expression is not array-indexable");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    if (!(array = get_var_deref(array)))
> > > +        return NULL;
> > > +
> > > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > > +        return NULL;
> > > +
> > > +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> > > +    deref->src.type = HLSL_IR_DEREF_ARRAY;
> > > +    deref->src.v.array.array = array;
> > > +    deref->src.v.array.index = index;
> > > +    return deref;
> > > +}
> >
> > It should be possible to have a sensible test for this one instead.
> > Care to write one?
>
> Inasmuch as expressions like (a + b)[i] are sensible, I guess...

Yep :D

[PATCH 3/5] d3dcompiler: Do not store matrix and vector indexes as deref instructions.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/hlsl.y | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 5875705cf18..cc868757a26 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -619,13 +619,10 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
     {
         data_type = expr_type->e.array.type;
     }
-    else if (expr_type->type == HLSL_CLASS_MATRIX)
+    else if (expr_type->type == HLSL_CLASS_MATRIX || expr_type->type == HLSL_CLASS_VECTOR)
     {
-        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
-    }
-    else if (expr_type->type == HLSL_CLASS_VECTOR)
-    {
-        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
+        FIXME("Index of matrix or vector type.\n");
+        return NULL;
     }
     else
     {
@@ -2337,9 +2334,6 @@ postfix_expr:             primary_expr
                             }
                         | postfix_expr '[' expr ']'
                             {
-                                /* This may be an array dereference or a vector/matrix
-                                 * subcomponent access.
-                                 * We store it as an array dereference in any case. */
                                 struct hlsl_ir_deref *deref;
 
                                 if (node_from_list($3)->data_type->type != HLSL_CLASS_SCALAR)
-- 
2.26.2

[PATCH 3/5] d3dcompiler: Do not store matrix and vector indexes as deref instructions.

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>

[PATCH 3/5] d3dcompiler: Do not store matrix and vector indexes as deref instructions.

[Matteo Bruni]

On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  dlls/d3dcompiler_43/hlsl.y | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index 5875705cf18..cc868757a26 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -619,13 +619,10 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
>      {
>          data_type = expr_type->e.array.type;
>      }
> -    else if (expr_type->type == HLSL_CLASS_MATRIX)
> +    else if (expr_type->type == HLSL_CLASS_MATRIX || expr_type->type == HLSL_CLASS_VECTOR)
>      {
> -        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> -    }
> -    else if (expr_type->type == HLSL_CLASS_VECTOR)
> -    {
> -        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> +        FIXME("Index of matrix or vector type.\n");
> +        return NULL;
>      }
>      else
>      {
> @@ -2337,9 +2334,6 @@ postfix_expr:             primary_expr
>                              }
>                          | postfix_expr '[' expr ']'
>                              {
> -                                /* This may be an array dereference or a vector/matrix
> -                                 * subcomponent access.
> -                                 * We store it as an array dereference in any case. */

Technically the first half of the comment still applies. Not sure it's
worth a comment but it didn't seem too obnoxious.

[PATCH 3/5] d3dcompiler: Do not store matrix and vector indexes as deref instructions.

[Zebediah Figura]

On 5/19/20 2:32 PM, Matteo Bruni wrote:
> On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> >  dlls/d3dcompiler_43/hlsl.y | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index 5875705cf18..cc868757a26 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -619,13 +619,10 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
> >      {
> >          data_type = expr_type->e.array.type;
> >      }
> > -    else if (expr_type->type == HLSL_CLASS_MATRIX)
> > +    else if (expr_type->type == HLSL_CLASS_MATRIX || expr_type->type == HLSL_CLASS_VECTOR)
> >      {
> > -        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> > -    }
> > -    else if (expr_type->type == HLSL_CLASS_VECTOR)
> > -    {
> > -        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> > +        FIXME("Index of matrix or vector type.\n");
> > +        return NULL;
> >      }
> >      else
> >      {
> > @@ -2337,9 +2334,6 @@ postfix_expr:             primary_expr
> >                              }
> >                          | postfix_expr '[' expr ']'
> >                              {
> > -                                /* This may be an array dereference or a vector/matrix
> > -                                 * subcomponent access.
> > -                                 * We store it as an array dereference in any case. */
>
> Technically the first half of the comment still applies. Not sure it's
> worth a comment but it didn't seem too obnoxious.

Sure. I think it's probably clear enough from new_array_deref(), but I
can restore the comment if you'd prefer.

[PATCH 3/5] d3dcompiler: Do not store matrix and vector indexes as deref instructions.

[Matteo Bruni]

On Tue, May 19, 2020 at 9:39 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 5/19/20 2:32 PM, Matteo Bruni wrote:
> > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > ---
> > >  dlls/d3dcompiler_43/hlsl.y | 12 +++---------
> > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > index 5875705cf18..cc868757a26 100644
> > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > @@ -619,13 +619,10 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
> > >      {
> > >          data_type = expr_type->e.array.type;
> > >      }
> > > -    else if (expr_type->type == HLSL_CLASS_MATRIX)
> > > +    else if (expr_type->type == HLSL_CLASS_MATRIX || expr_type->type == HLSL_CLASS_VECTOR)
> > >      {
> > > -        data_type = new_hlsl_type(NULL, HLSL_CLASS_VECTOR, expr_type->base_type, expr_type->dimx, 1);
> > > -    }
> > > -    else if (expr_type->type == HLSL_CLASS_VECTOR)
> > > -    {
> > > -        data_type = new_hlsl_type(NULL, HLSL_CLASS_SCALAR, expr_type->base_type, 1, 1);
> > > +        FIXME("Index of matrix or vector type.\n");
> > > +        return NULL;
> > >      }
> > >      else
> > >      {
> > > @@ -2337,9 +2334,6 @@ postfix_expr:             primary_expr
> > >                              }
> > >                          | postfix_expr '[' expr ']'
> > >                              {
> > > -                                /* This may be an array dereference or a vector/matrix
> > > -                                 * subcomponent access.
> > > -                                 * We store it as an array dereference in any case. */
> >
> > Technically the first half of the comment still applies. Not sure it's
> > worth a comment but it didn't seem too obnoxious.
>
> Sure. I think it's probably clear enough from new_array_deref(), but I
> can restore the comment if you'd prefer.

Nah, it's not worth the effort.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
One potential change to this patch is to always store an offset. Currently NULL
is used to signify an offset of 0, partly to save space, and partly because
otherwise new_var_deref() would need a bit of restructuring. But always using an
offset would simplify some code.

 dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
 dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
 dlls/d3dcompiler_43/utils.c               |  76 ++----------
 3 files changed, 97 insertions(+), 147 deletions(-)

diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 387e932a6cc..983057d543c 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
     DWORD swizzle;
 };
 
-enum hlsl_ir_deref_type
-{
-    HLSL_IR_DEREF_VAR,
-    HLSL_IR_DEREF_ARRAY,
-    HLSL_IR_DEREF_RECORD,
-};
-
 struct hlsl_deref
 {
-    enum hlsl_ir_deref_type type;
-    union
-    {
-        struct hlsl_ir_var *var;
-        struct
-        {
-            struct hlsl_ir_node *array;
-            struct hlsl_ir_node *index;
-        } array;
-        struct
-        {
-            struct hlsl_ir_node *record;
-            struct hlsl_struct_field *field;
-        } record;
-    } v;
+    struct hlsl_ir_var *var;
+    struct hlsl_ir_node *offset;
+    DWORD writemask;
 };
 
 struct hlsl_ir_deref
diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index cc868757a26..24157c8fdc9 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -538,15 +538,29 @@ static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs
         return NULL;
 
     init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
-    assign->lhs.type = HLSL_IR_DEREF_VAR;
-    assign->lhs.v.var = lhs;
+    assign->lhs.var = lhs;
     assign->rhs = rhs;
     if (type_is_single_reg(lhs->data_type))
-        assign->writemask = (1 << lhs->data_type->dimx) - 1;
+        assign->lhs.writemask = assign->writemask = (1 << lhs->data_type->dimx) - 1;
 
     return assign;
 }
 
+static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct source_location loc)
+{
+    struct hlsl_type *type;
+    struct hlsl_ir_constant *c;
+
+    if (!(type = new_hlsl_type(d3dcompiler_strdup("uint"), HLSL_CLASS_SCALAR, HLSL_TYPE_UINT, 1, 1)))
+        return NULL;
+
+    if (!(c = d3dcompiler_alloc(sizeof(*c))))
+        return NULL;
+    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
+    c->v.value.u[0] = n;
+    return c;
+}
+
 static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
 {
     struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
@@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
         return NULL;
     }
     init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
-    deref->src.type = HLSL_IR_DEREF_VAR;
-    deref->src.v.var = var;
+    deref->src.var = var;
+    if (type_is_single_reg(var->data_type))
+        deref->src.writemask = (1 << var->data_type->dimx) - 1;
     return deref;
 }
 
-static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
+static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
+        struct hlsl_type *data_type, const struct source_location loc)
 {
-    struct hlsl_ir_assignment *assign;
+    struct hlsl_ir_node *add = NULL;
     struct hlsl_ir_deref *deref;
     struct hlsl_ir_var *var;
-    char name[27];
 
-    if (node->type == HLSL_IR_DEREF)
-        return node;
+    if (var_node->type == HLSL_IR_DEREF)
+    {
+        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
 
-    sprintf(name, "<deref-%p>", node);
-    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
-        return NULL;
+        var = src->var;
+        if (src->offset)
+        {
+            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
+                return NULL;
+            list_add_after(&offset->entry, &add->entry);
+            offset = add;
+        }
+    }
+    else
+    {
+        struct hlsl_ir_assignment *assign;
+        char name[27];
+
+        sprintf(name, "<deref-%p>", var_node);
+        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
+            return NULL;
 
-    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
+        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
 
-    if (!(assign = make_simple_assignment(var, node)))
-        return NULL;
-    list_add_after(&node->entry, &assign->node.entry);
+        if (!(assign = make_simple_assignment(var, var_node)))
+            return NULL;
 
-    if (!(deref = new_var_deref(var, var->loc)))
+        list_add_after(&var_node->entry, &assign->node.entry);
+    }
+
+    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
         return NULL;
-    list_add_after(&assign->node.entry, &deref->node.entry);
-    return &deref->node;
+    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
+    deref->src.var = var;
+    deref->src.offset = offset;
+    if (type_is_single_reg(data_type))
+        deref->src.writemask = (1 << data_type->dimx) - 1;
+    list_add_after(&offset->entry, &deref->node.entry);
+    return deref;
 }
 
 static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
-        struct hlsl_struct_field *field, const struct source_location loc)
+        const struct hlsl_struct_field *field, const struct source_location loc)
 {
-    struct hlsl_ir_deref *deref;
+    struct hlsl_ir_constant *c;
 
-    if (!(record = get_var_deref(record)))
-        return NULL;
-
-    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
+    if (!(c = new_uint_constant(field->reg_offset, loc)))
         return NULL;
+    list_add_after(&record->entry, &c->node.entry);
 
-    init_node(&deref->node, HLSL_IR_DEREF, field->type, loc);
-    deref->src.type = HLSL_IR_DEREF_RECORD;
-    deref->src.v.record.record = record;
-    deref->src.v.record.field = field;
-    return deref;
+    return new_deref(record, &c->node, field->type, loc);
 }
 
 static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
         struct hlsl_ir_node *index, const struct source_location loc)
 {
     const struct hlsl_type *expr_type = array->data_type;
-    struct hlsl_ir_deref *deref;
     struct hlsl_type *data_type;
 
     TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
@@ -621,6 +651,7 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
     }
     else if (expr_type->type == HLSL_CLASS_MATRIX || expr_type->type == HLSL_CLASS_VECTOR)
     {
+        /* This needs to be lowered now, while we still have type information. */
         FIXME("Index of matrix or vector type.\n");
         return NULL;
     }
@@ -633,17 +664,21 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
         return NULL;
     }
 
-    if (!(array = get_var_deref(array)))
-        return NULL;
+    if (data_type->reg_size > 1)
+    {
+        struct hlsl_ir_constant *c;
+        struct hlsl_ir_node *mul;
 
-    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
-        return NULL;
+        if (!(c = new_uint_constant(data_type->reg_size, loc)))
+            return NULL;
+        list_add_after(&index->entry, &c->node.entry);
+        if (!(mul = new_binary_expr(HLSL_IR_BINOP_MUL, index, &c->node, loc)))
+            return NULL;
+        list_add_after(&c->node.entry, &mul->entry);
+        index = mul;
+    }
 
-    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
-    deref->src.type = HLSL_IR_DEREF_ARRAY;
-    deref->src.v.array.array = array;
-    deref->src.v.array.index = index;
-    return deref;
+    return new_deref(array, index, data_type, loc);
 }
 
 static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
@@ -2748,22 +2783,6 @@ static unsigned int index_instructions(struct list *instrs, unsigned int index)
     return index;
 }
 
-/* Walk the chain of derefs and retrieve the actual variable we care about. */
-static struct hlsl_ir_var *hlsl_var_from_deref(const struct hlsl_deref *deref)
-{
-    switch (deref->type)
-    {
-        case HLSL_IR_DEREF_VAR:
-            return deref->v.var;
-        case HLSL_IR_DEREF_ARRAY:
-            return hlsl_var_from_deref(&deref_from_node(deref->v.array.array)->src);
-        case HLSL_IR_DEREF_RECORD:
-            return hlsl_var_from_deref(&deref_from_node(deref->v.record.record)->src);
-    }
-    assert(0);
-    return NULL;
-}
-
 /* Compute the earliest and latest liveness for each variable. In the case that
  * a variable is accessed inside of a loop, we promote its liveness to extend
  * to at least the range of the entire loop. Note that we don't need to do this
@@ -2781,10 +2800,12 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
         case HLSL_IR_ASSIGNMENT:
         {
             struct hlsl_ir_assignment *assignment = assignment_from_node(instr);
-            var = hlsl_var_from_deref(&assignment->lhs);
+            var = assignment->lhs.var;
             if (!var->first_write)
                 var->first_write = loop_first ? min(instr->index, loop_first) : instr->index;
             assignment->rhs->last_read = instr->index;
+            if (assignment->lhs.offset)
+                assignment->lhs.offset->last_read = instr->index;
             break;
         }
         case HLSL_IR_CONSTANT:
@@ -2800,10 +2821,10 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
         case HLSL_IR_DEREF:
         {
             struct hlsl_ir_deref *deref = deref_from_node(instr);
-            var = hlsl_var_from_deref(&deref->src);
+            var = deref->src.var;
             var->last_read = loop_last ? max(instr->index, loop_last) : instr->index;
-            if (deref->src.type == HLSL_IR_DEREF_ARRAY)
-                deref->src.v.array.index->last_read = instr->index;
+            if (deref->src.offset)
+                deref->src.offset->last_read = instr->index;
             break;
         }
         case HLSL_IR_EXPR:
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index ac9da97f27c..4b1f99de65d 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -1437,29 +1437,6 @@ static unsigned int invert_swizzle(unsigned int *swizzle, unsigned int writemask
     return new_writemask;
 }
 
-static BOOL validate_lhs_deref(const struct hlsl_ir_node *lhs)
-{
-    struct hlsl_ir_deref *deref;
-
-    if (lhs->type != HLSL_IR_DEREF)
-    {
-        hlsl_report_message(lhs->loc, HLSL_LEVEL_ERROR, "invalid lvalue");
-        return FALSE;
-    }
-
-    deref = deref_from_node(lhs);
-
-    if (deref->src.type == HLSL_IR_DEREF_VAR)
-        return TRUE;
-    if (deref->src.type == HLSL_IR_DEREF_ARRAY)
-        return validate_lhs_deref(deref->src.v.array.array);
-    if (deref->src.type == HLSL_IR_DEREF_RECORD)
-        return validate_lhs_deref(deref->src.v.record.record);
-
-    assert(0);
-    return FALSE;
-}
-
 struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign_op assign_op,
         struct hlsl_ir_node *rhs)
 {
@@ -1513,12 +1490,6 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign
         lhs = lhs_inner;
     }
 
-    if (!validate_lhs_deref(lhs))
-    {
-        d3dcompiler_free(assign);
-        return NULL;
-    }
-
     TRACE("Creating proper assignment expression.\n");
     if (writemask == BWRITERSP_WRITEMASK_ALL)
         type = lhs->data_type;
@@ -1562,26 +1533,12 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign
         enum hlsl_ir_expr_op op = op_from_assignment(assign_op);
         struct hlsl_ir_node *expr;
 
-        if (assign->lhs.type != HLSL_IR_DEREF_VAR)
-        {
-            FIXME("LHS expression not supported in compound assignments yet.\n");
-            assign->rhs = rhs;
-        }
-        else
-        {
-            TRACE("Adding an expression for the compound assignment.\n");
-            expr = new_binary_expr(op, lhs, rhs, lhs->loc);
-            list_add_after(&rhs->entry, &expr->entry);
-            assign->rhs = expr;
-        }
-    }
-    else
-    {
-        list_remove(&lhs->entry);
-        /* Don't recursively free the deref; we just copied its members. */
-        d3dcompiler_free(lhs);
-        assign->rhs = rhs;
+        TRACE("Adding an expression for the compound assignment.\n");
+        expr = new_binary_expr(op, lhs, rhs, lhs->loc);
+        list_add_after(&rhs->entry, &expr->entry);
+        rhs = expr;
     }
+    assign->rhs = rhs;
 
     return &assign->node;
 }
@@ -1879,23 +1836,14 @@ static void debug_dump_ir_var(const struct hlsl_ir_var *var)
 
 static void debug_dump_deref(const struct hlsl_deref *deref)
 {
-    switch (deref->type)
+    wine_dbg_printf("deref(");
+    debug_dump_ir_var(deref->var);
+    wine_dbg_printf(")");
+    if (deref->offset)
     {
-        case HLSL_IR_DEREF_VAR:
-            wine_dbg_printf("deref(");
-            debug_dump_ir_var(deref->v.var);
-            wine_dbg_printf(")");
-            break;
-        case HLSL_IR_DEREF_ARRAY:
-            debug_dump_src(deref->v.array.array);
-            wine_dbg_printf("[");
-            debug_dump_src(deref->v.array.index);
-            wine_dbg_printf("]");
-            break;
-        case HLSL_IR_DEREF_RECORD:
-            debug_dump_src(deref->v.record.record);
-            wine_dbg_printf(".%s", debugstr_a(deref->v.record.field->name));
-            break;
+        wine_dbg_printf("[");
+        debug_dump_src(deref->offset);
+        wine_dbg_printf("]");
     }
 }
 
-- 
2.26.2

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Matteo Bruni]

On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
> One potential change to this patch is to always store an offset. Currently NULL
> is used to signify an offset of 0, partly to save space, and partly because
> otherwise new_var_deref() would need a bit of restructuring. But always using an
> offset would simplify some code.

Yeah, that might be a nice followup. Or, it feels like one, without
having actually touched the code at all...
> dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
>  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
>  dlls/d3dcompiler_43/utils.c               |  76 ++----------
>  3 files changed, 97 insertions(+), 147 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> index 387e932a6cc..983057d543c 100644
> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
>      DWORD swizzle;
>  };
>
> -enum hlsl_ir_deref_type
> -{
> -    HLSL_IR_DEREF_VAR,
> -    HLSL_IR_DEREF_ARRAY,
> -    HLSL_IR_DEREF_RECORD,
> -};
> -
>  struct hlsl_deref
>  {
> -    enum hlsl_ir_deref_type type;
> -    union
> -    {
> -        struct hlsl_ir_var *var;
> -        struct
> -        {
> -            struct hlsl_ir_node *array;
> -            struct hlsl_ir_node *index;
> -        } array;
> -        struct
> -        {
> -            struct hlsl_ir_node *record;
> -            struct hlsl_struct_field *field;
> -        } record;
> -    } v;
> +    struct hlsl_ir_var *var;
> +    struct hlsl_ir_node *offset;
> +    DWORD writemask;

I don't like moving the writemask into struct hlsl_deref. It's
something that makes sense only for an lvalue / assignment / STORE.
There are a few options here. Maybe turn struct hlsl_deref into struct
hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
the other use into struct hlsl_ir_deref while getting rid of the
writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
> };
>
> struct hlsl_ir_deref
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index cc868757a26..24157c8fdc9 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -538,15 +538,29 @@ static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs
>          return NULL;
>
> init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
> -    assign->lhs.type = HLSL_IR_DEREF_VAR;
> -    assign->lhs.v.var = lhs;
> +    assign->lhs.var = lhs;
>      assign->rhs = rhs;
>      if (type_is_single_reg(lhs->data_type))
> -        assign->writemask = (1 << lhs->data_type->dimx) - 1;
> +        assign->lhs.writemask = assign->writemask = (1 << lhs->data_type->dimx) - 1;
>
> return assign;
>  }
>
> +static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct source_location loc)
> +{
> +    struct hlsl_type *type;
> +    struct hlsl_ir_constant *c;
> +
> +    if (!(type = new_hlsl_type(d3dcompiler_strdup("uint"), HLSL_CLASS_SCALAR, HLSL_TYPE_UINT, 1, 1)))
> +        return NULL;

En passant: I was thinking that we could do something to avoid
redundantly creating hlsl_types over and over, at least for the
"basic" types. Probably something very simple, like storing each
scalar type in an array, would cover a lot already.
> +
> +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> +        return NULL;
> +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> +    c->v.value.u[0] = n;
> +    return c;
> +}

WRT memory usage, in particular when always storing the offset: we
could allocate only the required portion of the whole struct
hlsl_ir_constant i.e. something along the lines of
"d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
v.value.u[1]))". If the "tricky" allocation happens in a single place,
like a new_constant() helper or some-such, maybe it won't be too
ugly...
BTW I'm not sure that the rest of the compiler is ready for this kind
of thing. Let's call it another "nice to have at some point".
> +
>  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
>  {
>      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
>          return NULL;
>      }
>      init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
> -    deref->src.type = HLSL_IR_DEREF_VAR;
> -    deref->src.v.var = var;
> +    deref->src.var = var;
> +    if (type_is_single_reg(var->data_type))
> +        deref->src.writemask = (1 << var->data_type->dimx) - 1;
>      return deref;
>  }
>
> -static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
> +        struct hlsl_type *data_type, const struct source_location loc)
>  {
> -    struct hlsl_ir_assignment *assign;
> +    struct hlsl_ir_node *add = NULL;
>      struct hlsl_ir_deref *deref;
>      struct hlsl_ir_var *var;
> -    char name[27];
>
> -    if (node->type == HLSL_IR_DEREF)
> -        return node;
> +    if (var_node->type == HLSL_IR_DEREF)
> +    {
> +        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
>
> -    sprintf(name, "<deref-%p>", node);
> -    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> -        return NULL;
> +        var = src->var;
> +        if (src->offset)
> +        {
> +            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
> +                return NULL;
> +            list_add_after(&offset->entry, &add->entry);
> +            offset = add;
> +        }
> +    }
> +    else
> +    {
> +        struct hlsl_ir_assignment *assign;
> +        char name[27];
> +
> +        sprintf(name, "<deref-%p>", var_node);
> +        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
> +            return NULL;
>
> -    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
>
> -    if (!(assign = make_simple_assignment(var, node)))
> -        return NULL;
> -    list_add_after(&node->entry, &assign->node.entry);
> +        if (!(assign = make_simple_assignment(var, var_node)))
> +            return NULL;
>
> -    if (!(deref = new_var_deref(var, var->loc)))
> +        list_add_after(&var_node->entry, &assign->node.entry);
> +    }
> +
> +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
>          return NULL;
> -    list_add_after(&assign->node.entry, &deref->node.entry);
> -    return &deref->node;
> +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> +    deref->src.var = var;
> +    deref->src.offset = offset;
> +    if (type_is_single_reg(data_type))
> +        deref->src.writemask = (1 << data_type->dimx) - 1;
> +    list_add_after(&offset->entry, &deref->node.entry);
> +    return deref;
>  }

How bad is it to avoid creating "extra" deref nodes? Mostly curious,
it's okay to leave dead code around.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Zebediah Figura]

On 5/19/20 2:34 PM, Matteo Bruni wrote:
> On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> > One potential change to this patch is to always store an offset. Currently NULL
> > is used to signify an offset of 0, partly to save space, and partly because
> > otherwise new_var_deref() would need a bit of restructuring. But always using an
> > offset would simplify some code.
>
> Yeah, that might be a nice followup. Or, it feels like one, without
> having actually touched the code at all...
> > dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
> >  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
> >  dlls/d3dcompiler_43/utils.c               |  76 ++----------
> >  3 files changed, 97 insertions(+), 147 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > index 387e932a6cc..983057d543c 100644
> > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
> >      DWORD swizzle;
> >  };
> >
> > -enum hlsl_ir_deref_type
> > -{
> > -    HLSL_IR_DEREF_VAR,
> > -    HLSL_IR_DEREF_ARRAY,
> > -    HLSL_IR_DEREF_RECORD,
> > -};
> > -
> >  struct hlsl_deref
> >  {
> > -    enum hlsl_ir_deref_type type;
> > -    union
> > -    {
> > -        struct hlsl_ir_var *var;
> > -        struct
> > -        {
> > -            struct hlsl_ir_node *array;
> > -            struct hlsl_ir_node *index;
> > -        } array;
> > -        struct
> > -        {
> > -            struct hlsl_ir_node *record;
> > -            struct hlsl_struct_field *field;
> > -        } record;
> > -    } v;
> > +    struct hlsl_ir_var *var;
> > +    struct hlsl_ir_node *offset;
> > +    DWORD writemask;
>
> I don't like moving the writemask into struct hlsl_deref. It's
> something that makes sense only for an lvalue / assignment / STORE.
> There are a few options here. Maybe turn struct hlsl_deref into struct
> hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
> the other use into struct hlsl_ir_deref while getting rid of the
> writemask)? Do we lose a lot by "splitting" struct hlsl_deref?

"writemask" may be a bit of a misnomer, since there's not necessarily
writing (maybe "mask" is better?), but it's relevant for loads too. It's
essentially a companion to 'offset', to describe which part of the vec4
should be read from or written to. For loads we convert the mask into a
swizzle.
> > };
> >
> > struct hlsl_ir_deref
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index cc868757a26..24157c8fdc9 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -538,15 +538,29 @@ static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs
> >          return NULL;
> >
> > init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
> > -    assign->lhs.type = HLSL_IR_DEREF_VAR;
> > -    assign->lhs.v.var = lhs;
> > +    assign->lhs.var = lhs;
> >      assign->rhs = rhs;
> >      if (type_is_single_reg(lhs->data_type))
> > -        assign->writemask = (1 << lhs->data_type->dimx) - 1;
> > +        assign->lhs.writemask = assign->writemask = (1 << lhs->data_type->dimx) - 1;
> >
> > return assign;
> >  }
> >
> > +static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct source_location loc)
> > +{
> > +    struct hlsl_type *type;
> > +    struct hlsl_ir_constant *c;
> > +
> > +    if (!(type = new_hlsl_type(d3dcompiler_strdup("uint"), HLSL_CLASS_SCALAR, HLSL_TYPE_UINT, 1, 1)))
> > +        return NULL;
>
> En passant: I was thinking that we could do something to avoid
> redundantly creating hlsl_types over and over, at least for the
> "basic" types. Probably something very simple, like storing each
> scalar type in an array, would cover a lot already.

Yes, that's a good idea.
> > +
> > +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> > +        return NULL;
> > +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> > +    c->v.value.u[0] = n;
> > +    return c;
> > +}
>
> WRT memory usage, in particular when always storing the offset: we
> could allocate only the required portion of the whole struct
> hlsl_ir_constant i.e. something along the lines of
> "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
> v.value.u[1]))". If the "tricky" allocation happens in a single place,
> like a new_constant() helper or some-such, maybe it won't be too
> ugly...
> BTW I'm not sure that the rest of the compiler is ready for this kind
> of thing. Let's call it another "nice to have at some point".

This is potentially a good idea, but it also conflicts with one of two
possible ideas I had for a different problem:

I think we need some way of replacing instructions after parse time,
including with instructions of a different type. It's useful for
splitting, constant folding, lowering of one operation to another, and
so on.

One way to do this is to track def-use chains. This also lets us
simplify liveness analysis a bit, but it's not an awful lot of code that
we save that way.

The other thing that we could do is to make hlsl_ir_node a union. I
think this makes some code simpler, of course at the expense of using a
bit more memory.

Ultimately I don't have a strong preference, but it is something that'd
be nice to work out soon, as it does strongly affect the structure of
almost all code I subsequently write...
> > +
> >  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
> >  {
> >      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> > @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> >          return NULL;
> >      }
> >      init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
> > -    deref->src.type = HLSL_IR_DEREF_VAR;
> > -    deref->src.v.var = var;
> > +    deref->src.var = var;
> > +    if (type_is_single_reg(var->data_type))
> > +        deref->src.writemask = (1 << var->data_type->dimx) - 1;
> >      return deref;
> >  }
> >
> > -static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> > +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
> > +        struct hlsl_type *data_type, const struct source_location loc)
> >  {
> > -    struct hlsl_ir_assignment *assign;
> > +    struct hlsl_ir_node *add = NULL;
> >      struct hlsl_ir_deref *deref;
> >      struct hlsl_ir_var *var;
> > -    char name[27];
> >
> > -    if (node->type == HLSL_IR_DEREF)
> > -        return node;
> > +    if (var_node->type == HLSL_IR_DEREF)
> > +    {
> > +        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
> >
> > -    sprintf(name, "<deref-%p>", node);
> > -    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> > -        return NULL;
> > +        var = src->var;
> > +        if (src->offset)
> > +        {
> > +            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
> > +                return NULL;
> > +            list_add_after(&offset->entry, &add->entry);
> > +            offset = add;
> > +        }
> > +    }
> > +    else
> > +    {
> > +        struct hlsl_ir_assignment *assign;
> > +        char name[27];
> > +
> > +        sprintf(name, "<deref-%p>", var_node);
> > +        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
> > +            return NULL;
> >
> > -    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> > +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
> >
> > -    if (!(assign = make_simple_assignment(var, node)))
> > -        return NULL;
> > -    list_add_after(&node->entry, &assign->node.entry);
> > +        if (!(assign = make_simple_assignment(var, var_node)))
> > +            return NULL;
> >
> > -    if (!(deref = new_var_deref(var, var->loc)))
> > +        list_add_after(&var_node->entry, &assign->node.entry);
> > +    }
> > +
> > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> >          return NULL;
> > -    list_add_after(&assign->node.entry, &deref->node.entry);
> > -    return &deref->node;
> > +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> > +    deref->src.var = var;
> > +    deref->src.offset = offset;
> > +    if (type_is_single_reg(data_type))
> > +        deref->src.writemask = (1 << data_type->dimx) - 1;
> > +    list_add_after(&offset->entry, &deref->node.entry);
> > +    return deref;
> >  }
>
> How bad is it to avoid creating "extra" deref nodes? Mostly curious,
> it's okay to leave dead code around.

So I'm guessing that what you mean by this is: given an expression like
"a.b.c", instead of

2: a
3: 2 # offset of b
4: a[@3]
5: 2 # offset of c
6: @3 + @5
7: a[@6]

we delete or replace @2 and @4 when generating @4 and @7, respectively.

It's a bit tricky, because while in practice I think that instructions
@2 and @4 will have no uses, that's not really spelled out as a
guarantee anywhere, and having it be a thing to implicitly remember
makes me a *little* nervous. (I'm not sure I trust myself to implicitly
remember it.) If we track def-use chains, that's less of a problem, but
I'm still not sure whether we want to track def-use chains—see above.

(Arguably we need def-use anyway, for DCE, but we don't really—we only
need to check whether a field is used, not where, so we can just reuse
the liveness analysis passes we already have.)

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Matteo Bruni]

On Tue, May 19, 2020 at 10:11 PM Zebediah Figura
<zfigura(a)codeweavers.com> wrote:
> On 5/19/20 2:34 PM, Matteo Bruni wrote:
> > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > ---
> > > One potential change to this patch is to always store an offset. Currently NULL
> > > is used to signify an offset of 0, partly to save space, and partly because
> > > otherwise new_var_deref() would need a bit of restructuring. But always using an
> > > offset would simplify some code.
> >
> > Yeah, that might be a nice followup. Or, it feels like one, without
> > having actually touched the code at all...
> > > dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
> > >  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
> > >  dlls/d3dcompiler_43/utils.c               |  76 ++----------
> > >  3 files changed, 97 insertions(+), 147 deletions(-)
> > >
> > > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > index 387e932a6cc..983057d543c 100644
> > > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
> > >      DWORD swizzle;
> > >  };
> > >
> > > -enum hlsl_ir_deref_type
> > > -{
> > > -    HLSL_IR_DEREF_VAR,
> > > -    HLSL_IR_DEREF_ARRAY,
> > > -    HLSL_IR_DEREF_RECORD,
> > > -};
> > > -
> > >  struct hlsl_deref
> > >  {
> > > -    enum hlsl_ir_deref_type type;
> > > -    union
> > > -    {
> > > -        struct hlsl_ir_var *var;
> > > -        struct
> > > -        {
> > > -            struct hlsl_ir_node *array;
> > > -            struct hlsl_ir_node *index;
> > > -        } array;
> > > -        struct
> > > -        {
> > > -            struct hlsl_ir_node *record;
> > > -            struct hlsl_struct_field *field;
> > > -        } record;
> > > -    } v;
> > > +    struct hlsl_ir_var *var;
> > > +    struct hlsl_ir_node *offset;
> > > +    DWORD writemask;
> >
> > I don't like moving the writemask into struct hlsl_deref. It's
> > something that makes sense only for an lvalue / assignment / STORE.
> > There are a few options here. Maybe turn struct hlsl_deref into struct
> > hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
> > the other use into struct hlsl_ir_deref while getting rid of the
> > writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
>
> "writemask" may be a bit of a misnomer, since there's not necessarily
> writing (maybe "mask" is better?), but it's relevant for loads too. It's
> essentially a companion to 'offset', to describe which part of the vec4
> should be read from or written to. For loads we convert the mask into a
> swizzle.

It shouldn't be necessary though, if the offset is in the unit of
(scalar) components rather than 4-component registers. Per-component
offset, together with the data type should be enough.
Now, of course I haven't really thought it through so there might be dragons...
> > > +
> > > +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> > > +        return NULL;
> > > +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> > > +    c->v.value.u[0] = n;
> > > +    return c;
> > > +}
> >
> > WRT memory usage, in particular when always storing the offset: we
> > could allocate only the required portion of the whole struct
> > hlsl_ir_constant i.e. something along the lines of
> > "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
> > v.value.u[1]))". If the "tricky" allocation happens in a single place,
> > like a new_constant() helper or some-such, maybe it won't be too
> > ugly...
> > BTW I'm not sure that the rest of the compiler is ready for this kind
> > of thing. Let's call it another "nice to have at some point".
>
> This is potentially a good idea, but it also conflicts with one of two
> possible ideas I had for a different problem:
>
> I think we need some way of replacing instructions after parse time,
> including with instructions of a different type. It's useful for
> splitting, constant folding, lowering of one operation to another, and
> so on.
>
> One way to do this is to track def-use chains. This also lets us
> simplify liveness analysis a bit, but it's not an awful lot of code that
> we save that way.
>
> The other thing that we could do is to make hlsl_ir_node a union. I
> think this makes some code simpler, of course at the expense of using a
> bit more memory.
>
> Ultimately I don't have a strong preference, but it is something that'd
> be nice to work out soon, as it does strongly affect the structure of
> almost all code I subsequently write...

So your idea is to replace an instruction with a different one "in
place" so that all the pointers that refer to it don't need to be
updated. My gut feeling is that it might not be enough in the general
case. In particular, if you split an instruction into multiple
instructions (e.g. maybe some matrix operation into per-vector
arithmetic) you might want to point a users of the original
instruction to any of the new ones.
This kind of transformation would break "plain" def-use chain
replacement too, but that seems solvable.
> > > +
> > >  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
> > >  {
> > >      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> > > @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> > >          return NULL;
> > >      }
> > >      init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
> > > -    deref->src.type = HLSL_IR_DEREF_VAR;
> > > -    deref->src.v.var = var;
> > > +    deref->src.var = var;
> > > +    if (type_is_single_reg(var->data_type))
> > > +        deref->src.writemask = (1 << var->data_type->dimx) - 1;
> > >      return deref;
> > >  }
> > >
> > > -static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> > > +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
> > > +        struct hlsl_type *data_type, const struct source_location loc)
> > >  {
> > > -    struct hlsl_ir_assignment *assign;
> > > +    struct hlsl_ir_node *add = NULL;
> > >      struct hlsl_ir_deref *deref;
> > >      struct hlsl_ir_var *var;
> > > -    char name[27];
> > >
> > > -    if (node->type == HLSL_IR_DEREF)
> > > -        return node;
> > > +    if (var_node->type == HLSL_IR_DEREF)
> > > +    {
> > > +        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
> > >
> > > -    sprintf(name, "<deref-%p>", node);
> > > -    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> > > -        return NULL;
> > > +        var = src->var;
> > > +        if (src->offset)
> > > +        {
> > > +            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
> > > +                return NULL;
> > > +            list_add_after(&offset->entry, &add->entry);
> > > +            offset = add;
> > > +        }
> > > +    }
> > > +    else
> > > +    {
> > > +        struct hlsl_ir_assignment *assign;
> > > +        char name[27];
> > > +
> > > +        sprintf(name, "<deref-%p>", var_node);
> > > +        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
> > > +            return NULL;
> > >
> > > -    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> > > +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
> > >
> > > -    if (!(assign = make_simple_assignment(var, node)))
> > > -        return NULL;
> > > -    list_add_after(&node->entry, &assign->node.entry);
> > > +        if (!(assign = make_simple_assignment(var, var_node)))
> > > +            return NULL;
> > >
> > > -    if (!(deref = new_var_deref(var, var->loc)))
> > > +        list_add_after(&var_node->entry, &assign->node.entry);
> > > +    }
> > > +
> > > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > >          return NULL;
> > > -    list_add_after(&assign->node.entry, &deref->node.entry);
> > > -    return &deref->node;
> > > +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> > > +    deref->src.var = var;
> > > +    deref->src.offset = offset;
> > > +    if (type_is_single_reg(data_type))
> > > +        deref->src.writemask = (1 << data_type->dimx) - 1;
> > > +    list_add_after(&offset->entry, &deref->node.entry);
> > > +    return deref;
> > >  }
> >
> > How bad is it to avoid creating "extra" deref nodes? Mostly curious,
> > it's okay to leave dead code around.
>
> So I'm guessing that what you mean by this is: given an expression like
> "a.b.c", instead of
>
> 2: a
> 3: 2 # offset of b
> 4: a[@3]
> 5: 2 # offset of c
> 6: @3 + @5
> 7: a[@6]
>
> we delete or replace @2 and @4 when generating @4 and @7, respectively.
>
> It's a bit tricky, because while in practice I think that instructions
> @2 and @4 will have no uses, that's not really spelled out as a
> guarantee anywhere, and having it be a thing to implicitly remember
> makes me a *little* nervous. (I'm not sure I trust myself to implicitly
> remember it.) If we track def-use chains, that's less of a problem, but
> I'm still not sure whether we want to track def-use chains—see above.
>
> (Arguably we need def-use anyway, for DCE, but we don't really—we only
> need to check whether a field is used, not where, so we can just reuse
> the liveness analysis passes we already have.)

Yes, that was my question, and yeah, I see. Definitely no point in
complicating this for no reason aside from making the IR nicer to
read.

Also agreed, the current liveness stuff should be enough for DCE,
especially for the time being.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Zebediah Figura]

On 5/19/20 3:44 PM, Matteo Bruni wrote:
> On Tue, May 19, 2020 at 10:11 PM Zebediah Figura
> <zfigura(a)codeweavers.com> wrote:
> > On 5/19/20 2:34 PM, Matteo Bruni wrote:
> > > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > > ---
> > > > One potential change to this patch is to always store an offset. Currently NULL
> > > > is used to signify an offset of 0, partly to save space, and partly because
> > > > otherwise new_var_deref() would need a bit of restructuring. But always using an
> > > > offset would simplify some code.
> > >
> > > Yeah, that might be a nice followup. Or, it feels like one, without
> > > having actually touched the code at all...
> > > > dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
> > > >  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
> > > >  dlls/d3dcompiler_43/utils.c               |  76 ++----------
> > > >  3 files changed, 97 insertions(+), 147 deletions(-)
> > > >
> > > > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > index 387e932a6cc..983057d543c 100644
> > > > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
> > > >      DWORD swizzle;
> > > >  };
> > > >
> > > > -enum hlsl_ir_deref_type
> > > > -{
> > > > -    HLSL_IR_DEREF_VAR,
> > > > -    HLSL_IR_DEREF_ARRAY,
> > > > -    HLSL_IR_DEREF_RECORD,
> > > > -};
> > > > -
> > > >  struct hlsl_deref
> > > >  {
> > > > -    enum hlsl_ir_deref_type type;
> > > > -    union
> > > > -    {
> > > > -        struct hlsl_ir_var *var;
> > > > -        struct
> > > > -        {
> > > > -            struct hlsl_ir_node *array;
> > > > -            struct hlsl_ir_node *index;
> > > > -        } array;
> > > > -        struct
> > > > -        {
> > > > -            struct hlsl_ir_node *record;
> > > > -            struct hlsl_struct_field *field;
> > > > -        } record;
> > > > -    } v;
> > > > +    struct hlsl_ir_var *var;
> > > > +    struct hlsl_ir_node *offset;
> > > > +    DWORD writemask;
> > >
> > > I don't like moving the writemask into struct hlsl_deref. It's
> > > something that makes sense only for an lvalue / assignment / STORE.
> > > There are a few options here. Maybe turn struct hlsl_deref into struct
> > > hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
> > > the other use into struct hlsl_ir_deref while getting rid of the
> > > writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
> >
> > "writemask" may be a bit of a misnomer, since there's not necessarily
> > writing (maybe "mask" is better?), but it's relevant for loads too. It's
> > essentially a companion to 'offset', to describe which part of the vec4
> > should be read from or written to. For loads we convert the mask into a
> > swizzle.
>
> It shouldn't be necessary though, if the offset is in the unit of
> (scalar) components rather than 4-component registers. Per-component
> offset, together with the data type should be enough.
> Now, of course I haven't really thought it through so there might be dragons...

Yes, you're right. I've been dealing with the offset in registers, but
there's not really a good reason to do that...
> > > > +
> > > > +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> > > > +        return NULL;
> > > > +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> > > > +    c->v.value.u[0] = n;
> > > > +    return c;
> > > > +}
> > >
> > > WRT memory usage, in particular when always storing the offset: we
> > > could allocate only the required portion of the whole struct
> > > hlsl_ir_constant i.e. something along the lines of
> > > "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
> > > v.value.u[1]))". If the "tricky" allocation happens in a single place,
> > > like a new_constant() helper or some-such, maybe it won't be too
> > > ugly...
> > > BTW I'm not sure that the rest of the compiler is ready for this kind
> > > of thing. Let's call it another "nice to have at some point".
> >
> > This is potentially a good idea, but it also conflicts with one of two
> > possible ideas I had for a different problem:
> >
> > I think we need some way of replacing instructions after parse time,
> > including with instructions of a different type. It's useful for
> > splitting, constant folding, lowering of one operation to another, and
> > so on.
> >
> > One way to do this is to track def-use chains. This also lets us
> > simplify liveness analysis a bit, but it's not an awful lot of code that
> > we save that way.
> >
> > The other thing that we could do is to make hlsl_ir_node a union. I
> > think this makes some code simpler, of course at the expense of using a
> > bit more memory.
> >
> > Ultimately I don't have a strong preference, but it is something that'd
> > be nice to work out soon, as it does strongly affect the structure of
> > almost all code I subsequently write...
>
> So your idea is to replace an instruction with a different one "in
> place" so that all the pointers that refer to it don't need to be
> updated. My gut feeling is that it might not be enough in the general
> case. In particular, if you split an instruction into multiple
> instructions (e.g. maybe some matrix operation into per-vector
> arithmetic) you might want to point a users of the original
> instruction to any of the new ones.
> This kind of transformation would break "plain" def-use chain
> replacement too, but that seems solvable.

I think that in any such case we also have to replace the new
instruction, though. Put another way, if an instruction consumes a
matrix, then it still needs to consume a matrix even once the
instruction producing a matrix has been split to produce vectors. I
don't think it ever makes sense to change the source of an instruction
to be a different type.

To try to express how I'm planning to split things, given an expression
like "mat1 = mat2 * 2" we generate

2: load(mat2)
3: 2
4: @2 * @3
5: mat1 = @4

We split @4, yielding

 2: load(mat2)
 3: 2
 4: @2 * @3
 5: load(mat2[0])
 6: @5 * @3
 7: <syn-4>[0] = @6
 8: load(mat2[1])
 9: @8 * @3
10: <syn-4>[1] = @9
11: mat1 = @4

We split @11, yielding

    ...
10: <syn-4>[1] = @9
11: mat1 = @4
12: load(<syn-4>[0])
13: mat1[0] = @12
14: load(<syn-4>[1])
15: mat1[1] = @14

@11 is removed after splitting, and @4 is subsequently dce'd out. The
crucially relevant bit is that we keep both instructions around after
splitting them, and we don't actually change the source. This was the
simplest approach I could come up with, because it means we can
essentially split one instruction at a time, without having to recurse.
> > > > +
> > > >  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
> > > >  {
> > > >      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> > > > @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> > > >          return NULL;
> > > >      }
> > > >      init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
> > > > -    deref->src.type = HLSL_IR_DEREF_VAR;
> > > > -    deref->src.v.var = var;
> > > > +    deref->src.var = var;
> > > > +    if (type_is_single_reg(var->data_type))
> > > > +        deref->src.writemask = (1 << var->data_type->dimx) - 1;
> > > >      return deref;
> > > >  }
> > > >
> > > > -static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node)
> > > > +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
> > > > +        struct hlsl_type *data_type, const struct source_location loc)
> > > >  {
> > > > -    struct hlsl_ir_assignment *assign;
> > > > +    struct hlsl_ir_node *add = NULL;
> > > >      struct hlsl_ir_deref *deref;
> > > >      struct hlsl_ir_var *var;
> > > > -    char name[27];
> > > >
> > > > -    if (node->type == HLSL_IR_DEREF)
> > > > -        return node;
> > > > +    if (var_node->type == HLSL_IR_DEREF)
> > > > +    {
> > > > +        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
> > > >
> > > > -    sprintf(name, "<deref-%p>", node);
> > > > -    if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
> > > > -        return NULL;
> > > > +        var = src->var;
> > > > +        if (src->offset)
> > > > +        {
> > > > +            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
> > > > +                return NULL;
> > > > +            list_add_after(&offset->entry, &add->entry);
> > > > +            offset = add;
> > > > +        }
> > > > +    }
> > > > +    else
> > > > +    {
> > > > +        struct hlsl_ir_assignment *assign;
> > > > +        char name[27];
> > > > +
> > > > +        sprintf(name, "<deref-%p>", var_node);
> > > > +        if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))
> > > > +            return NULL;
> > > >
> > > > -    TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
> > > > +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
> > > >
> > > > -    if (!(assign = make_simple_assignment(var, node)))
> > > > -        return NULL;
> > > > -    list_add_after(&node->entry, &assign->node.entry);
> > > > +        if (!(assign = make_simple_assignment(var, var_node)))
> > > > +            return NULL;
> > > >
> > > > -    if (!(deref = new_var_deref(var, var->loc)))
> > > > +        list_add_after(&var_node->entry, &assign->node.entry);
> > > > +    }
> > > > +
> > > > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > > >          return NULL;
> > > > -    list_add_after(&assign->node.entry, &deref->node.entry);
> > > > -    return &deref->node;
> > > > +    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
> > > > +    deref->src.var = var;
> > > > +    deref->src.offset = offset;
> > > > +    if (type_is_single_reg(data_type))
> > > > +        deref->src.writemask = (1 << data_type->dimx) - 1;
> > > > +    list_add_after(&offset->entry, &deref->node.entry);
> > > > +    return deref;
> > > >  }
> > >
> > > How bad is it to avoid creating "extra" deref nodes? Mostly curious,
> > > it's okay to leave dead code around.
> >
> > So I'm guessing that what you mean by this is: given an expression like
> > "a.b.c", instead of
> >
> > 2: a
> > 3: 2 # offset of b
> > 4: a[@3]
> > 5: 2 # offset of c
> > 6: @3 + @5
> > 7: a[@6]
> >
> > we delete or replace @2 and @4 when generating @4 and @7, respectively.
> >
> > It's a bit tricky, because while in practice I think that instructions
> > @2 and @4 will have no uses, that's not really spelled out as a
> > guarantee anywhere, and having it be a thing to implicitly remember
> > makes me a *little* nervous. (I'm not sure I trust myself to implicitly
> > remember it.) If we track def-use chains, that's less of a problem, but
> > I'm still not sure whether we want to track def-use chains—see above.
> >
> > (Arguably we need def-use anyway, for DCE, but we don't really—we only
> > need to check whether a field is used, not where, so we can just reuse
> > the liveness analysis passes we already have.)
>
> Yes, that was my question, and yeah, I see. Definitely no point in
> complicating this for no reason aside from making the IR nicer to
> read.
>
> Also agreed, the current liveness stuff should be enough for DCE,
> especially for the time being.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Matteo Bruni]

On Tue, May 19, 2020 at 11:14 PM Zebediah Figura
<zfigura(a)codeweavers.com> wrote:
> On 5/19/20 3:44 PM, Matteo Bruni wrote:
> > On Tue, May 19, 2020 at 10:11 PM Zebediah Figura
> > <zfigura(a)codeweavers.com> wrote:
> > > On 5/19/20 2:34 PM, Matteo Bruni wrote:
> > > > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > > > ---
> > > > > One potential change to this patch is to always store an offset. Currently NULL
> > > > > is used to signify an offset of 0, partly to save space, and partly because
> > > > > otherwise new_var_deref() would need a bit of restructuring. But always using an
> > > > > offset would simplify some code.
> > > >
> > > > Yeah, that might be a nice followup. Or, it feels like one, without
> > > > having actually touched the code at all...
> > > > > dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
> > > > >  dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
> > > > >  dlls/d3dcompiler_43/utils.c               |  76 ++----------
> > > > >  3 files changed, 97 insertions(+), 147 deletions(-)
> > > > >
> > > > > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > index 387e932a6cc..983057d543c 100644
> > > > > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
> > > > >      DWORD swizzle;
> > > > >  };
> > > > >
> > > > > -enum hlsl_ir_deref_type
> > > > > -{
> > > > > -    HLSL_IR_DEREF_VAR,
> > > > > -    HLSL_IR_DEREF_ARRAY,
> > > > > -    HLSL_IR_DEREF_RECORD,
> > > > > -};
> > > > > -
> > > > >  struct hlsl_deref
> > > > >  {
> > > > > -    enum hlsl_ir_deref_type type;
> > > > > -    union
> > > > > -    {
> > > > > -        struct hlsl_ir_var *var;
> > > > > -        struct
> > > > > -        {
> > > > > -            struct hlsl_ir_node *array;
> > > > > -            struct hlsl_ir_node *index;
> > > > > -        } array;
> > > > > -        struct
> > > > > -        {
> > > > > -            struct hlsl_ir_node *record;
> > > > > -            struct hlsl_struct_field *field;
> > > > > -        } record;
> > > > > -    } v;
> > > > > +    struct hlsl_ir_var *var;
> > > > > +    struct hlsl_ir_node *offset;
> > > > > +    DWORD writemask;
> > > >
> > > > I don't like moving the writemask into struct hlsl_deref. It's
> > > > something that makes sense only for an lvalue / assignment / STORE.
> > > > There are a few options here. Maybe turn struct hlsl_deref into struct
> > > > hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
> > > > the other use into struct hlsl_ir_deref while getting rid of the
> > > > writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
> > >
> > > "writemask" may be a bit of a misnomer, since there's not necessarily
> > > writing (maybe "mask" is better?), but it's relevant for loads too. It's
> > > essentially a companion to 'offset', to describe which part of the vec4
> > > should be read from or written to. For loads we convert the mask into a
> > > swizzle.
> >
> > It shouldn't be necessary though, if the offset is in the unit of
> > (scalar) components rather than 4-component registers. Per-component
> > offset, together with the data type should be enough.
> > Now, of course I haven't really thought it through so there might be dragons...
>
> Yes, you're right. I've been dealing with the offset in registers, but
> there's not really a good reason to do that...
> > > > > +
> > > > > +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> > > > > +        return NULL;
> > > > > +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> > > > > +    c->v.value.u[0] = n;
> > > > > +    return c;
> > > > > +}
> > > >
> > > > WRT memory usage, in particular when always storing the offset: we
> > > > could allocate only the required portion of the whole struct
> > > > hlsl_ir_constant i.e. something along the lines of
> > > > "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
> > > > v.value.u[1]))". If the "tricky" allocation happens in a single place,
> > > > like a new_constant() helper or some-such, maybe it won't be too
> > > > ugly...
> > > > BTW I'm not sure that the rest of the compiler is ready for this kind
> > > > of thing. Let's call it another "nice to have at some point".
> > >
> > > This is potentially a good idea, but it also conflicts with one of two
> > > possible ideas I had for a different problem:
> > >
> > > I think we need some way of replacing instructions after parse time,
> > > including with instructions of a different type. It's useful for
> > > splitting, constant folding, lowering of one operation to another, and
> > > so on.
> > >
> > > One way to do this is to track def-use chains. This also lets us
> > > simplify liveness analysis a bit, but it's not an awful lot of code that
> > > we save that way.
> > >
> > > The other thing that we could do is to make hlsl_ir_node a union. I
> > > think this makes some code simpler, of course at the expense of using a
> > > bit more memory.
> > >
> > > Ultimately I don't have a strong preference, but it is something that'd
> > > be nice to work out soon, as it does strongly affect the structure of
> > > almost all code I subsequently write...
> >
> > So your idea is to replace an instruction with a different one "in
> > place" so that all the pointers that refer to it don't need to be
> > updated. My gut feeling is that it might not be enough in the general
> > case. In particular, if you split an instruction into multiple
> > instructions (e.g. maybe some matrix operation into per-vector
> > arithmetic) you might want to point a users of the original
> > instruction to any of the new ones.
> > This kind of transformation would break "plain" def-use chain
> > replacement too, but that seems solvable.
>
> I think that in any such case we also have to replace the new
> instruction, though. Put another way, if an instruction consumes a
> matrix, then it still needs to consume a matrix even once the
> instruction producing a matrix has been split to produce vectors. I
> don't think it ever makes sense to change the source of an instruction
> to be a different type.
>
> To try to express how I'm planning to split things, given an expression
> like "mat1 = mat2 * 2" we generate
>
> 2: load(mat2)
> 3: 2
> 4: @2 * @3
> 5: mat1 = @4
>
> We split @4, yielding
>
> 2: load(mat2)
>  3: 2
>  4: @2 * @3
>  5: load(mat2[0])
>  6: @5 * @3
>  7: <syn-4>[0] = @6
>  8: load(mat2[1])
>  9: @8 * @3
> 10: <syn-4>[1] = @9
> 11: mat1 = @4
>
> We split @11, yielding
>
> ...
> 10: <syn-4>[1] = @9
> 11: mat1 = @4
> 12: load(<syn-4>[0])
> 13: mat1[0] = @12
> 14: load(<syn-4>[1])
> 15: mat1[1] = @14
>
> @11 is removed after splitting, and @4 is subsequently dce'd out. The
> crucially relevant bit is that we keep both instructions around after
> splitting them, and we don't actually change the source. This was the
> simplest approach I could come up with, because it means we can
> essentially split one instruction at a time, without having to recurse.

Cool. Yeah, that looks simpler than what I was envisioning. I'm not
sure how turning struct hlsl_ir_node into a union would help, or
matter, though.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Zebediah Figura]

On 5/19/20 5:07 PM, Matteo Bruni wrote:
> On Tue, May 19, 2020 at 11:14 PM Zebediah Figura
> <zfigura(a)codeweavers.com> wrote:
> > On 5/19/20 3:44 PM, Matteo Bruni wrote:
> > > On Tue, May 19, 2020 at 10:11 PM Zebediah Figura
> > > <zfigura(a)codeweavers.com> wrote:
> > > > On 5/19/20 2:34 PM, Matteo Bruni wrote:
> > > > > On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > > > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > > > > ---
> > > > > > One potential change to this patch is to always store an offset. Currently NULL
> > > > > > is used to signify an offset of 0, partly to save space, and partly because
> > > > > > otherwise new_var_deref() would need a bit of restructuring. But always using an
> > > > > > offset would simplify some code.
> > > > >
> > > > > Yeah, that might be a nice followup. Or, it feels like one, without
> > > > > having actually touched the code at all...
> > > > > > dlls/d3dcompiler_43/d3dcompiler_private.h |  25 +---
> > > > > >   dlls/d3dcompiler_43/hlsl.y                | 143 +++++++++++++---------
> > > > > >   dlls/d3dcompiler_43/utils.c               |  76 ++----------
> > > > > >   3 files changed, 97 insertions(+), 147 deletions(-)
> > > > > >
> > > > > > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > > index 387e932a6cc..983057d543c 100644
> > > > > > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > > > > @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle
> > > > > >       DWORD swizzle;
> > > > > >   };
> > > > > >
> > > > > > -enum hlsl_ir_deref_type
> > > > > > -{
> > > > > > -    HLSL_IR_DEREF_VAR,
> > > > > > -    HLSL_IR_DEREF_ARRAY,
> > > > > > -    HLSL_IR_DEREF_RECORD,
> > > > > > -};
> > > > > > -
> > > > > >   struct hlsl_deref
> > > > > >   {
> > > > > > -    enum hlsl_ir_deref_type type;
> > > > > > -    union
> > > > > > -    {
> > > > > > -        struct hlsl_ir_var *var;
> > > > > > -        struct
> > > > > > -        {
> > > > > > -            struct hlsl_ir_node *array;
> > > > > > -            struct hlsl_ir_node *index;
> > > > > > -        } array;
> > > > > > -        struct
> > > > > > -        {
> > > > > > -            struct hlsl_ir_node *record;
> > > > > > -            struct hlsl_struct_field *field;
> > > > > > -        } record;
> > > > > > -    } v;
> > > > > > +    struct hlsl_ir_var *var;
> > > > > > +    struct hlsl_ir_node *offset;
> > > > > > +    DWORD writemask;
> > > > >
> > > > > I don't like moving the writemask into struct hlsl_deref. It's
> > > > > something that makes sense only for an lvalue / assignment / STORE.
> > > > > There are a few options here. Maybe turn struct hlsl_deref into struct
> > > > > hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding
> > > > > the other use into struct hlsl_ir_deref while getting rid of the
> > > > > writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
> > > >
> > > > "writemask" may be a bit of a misnomer, since there's not necessarily
> > > > writing (maybe "mask" is better?), but it's relevant for loads too. It's
> > > > essentially a companion to 'offset', to describe which part of the vec4
> > > > should be read from or written to. For loads we convert the mask into a
> > > > swizzle.
> > >
> > > It shouldn't be necessary though, if the offset is in the unit of
> > > (scalar) components rather than 4-component registers. Per-component
> > > offset, together with the data type should be enough.
> > > Now, of course I haven't really thought it through so there might be dragons...
> >
> > Yes, you're right. I've been dealing with the offset in registers, but
> > there's not really a good reason to do that...
> > > > > > +
> > > > > > +    if (!(c = d3dcompiler_alloc(sizeof(*c))))
> > > > > > +        return NULL;
> > > > > > +    init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
> > > > > > +    c->v.value.u[0] = n;
> > > > > > +    return c;
> > > > > > +}
> > > > >
> > > > > WRT memory usage, in particular when always storing the offset: we
> > > > > could allocate only the required portion of the whole struct
> > > > > hlsl_ir_constant i.e. something along the lines of
> > > > > "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant,
> > > > > v.value.u[1]))". If the "tricky" allocation happens in a single place,
> > > > > like a new_constant() helper or some-such, maybe it won't be too
> > > > > ugly...
> > > > > BTW I'm not sure that the rest of the compiler is ready for this kind
> > > > > of thing. Let's call it another "nice to have at some point".
> > > >
> > > > This is potentially a good idea, but it also conflicts with one of two
> > > > possible ideas I had for a different problem:
> > > >
> > > > I think we need some way of replacing instructions after parse time,
> > > > including with instructions of a different type. It's useful for
> > > > splitting, constant folding, lowering of one operation to another, and
> > > > so on.
> > > >
> > > > One way to do this is to track def-use chains. This also lets us
> > > > simplify liveness analysis a bit, but it's not an awful lot of code that
> > > > we save that way.
> > > >
> > > > The other thing that we could do is to make hlsl_ir_node a union. I
> > > > think this makes some code simpler, of course at the expense of using a
> > > > bit more memory.
> > > >
> > > > Ultimately I don't have a strong preference, but it is something that'd
> > > > be nice to work out soon, as it does strongly affect the structure of
> > > > almost all code I subsequently write...
> > >
> > > So your idea is to replace an instruction with a different one "in
> > > place" so that all the pointers that refer to it don't need to be
> > > updated. My gut feeling is that it might not be enough in the general
> > > case. In particular, if you split an instruction into multiple
> > > instructions (e.g. maybe some matrix operation into per-vector
> > > arithmetic) you might want to point a users of the original
> > > instruction to any of the new ones.
> > > This kind of transformation would break "plain" def-use chain
> > > replacement too, but that seems solvable.
> >
> > I think that in any such case we also have to replace the new
> > instruction, though. Put another way, if an instruction consumes a
> > matrix, then it still needs to consume a matrix even once the
> > instruction producing a matrix has been split to produce vectors. I
> > don't think it ever makes sense to change the source of an instruction
> > to be a different type.
> >
> > To try to express how I'm planning to split things, given an expression
> > like "mat1 = mat2 * 2" we generate
> >
> > 2: load(mat2)
> > 3: 2
> > 4: @2 * @3
> > 5: mat1 = @4
> >
> > We split @4, yielding
> >
> > 2: load(mat2)
> >   3: 2
> >   4: @2 * @3
> >   5: load(mat2[0])
> >   6: @5 * @3
> >   7: <syn-4>[0] = @6
> >   8: load(mat2[1])
> >   9: @8 * @3
> > 10: <syn-4>[1] = @9
> > 11: mat1 = @4
> >
> > We split @11, yielding
> >
> > ...
> > 10: <syn-4>[1] = @9
> > 11: mat1 = @4
> > 12: load(<syn-4>[0])
> > 13: mat1[0] = @12
> > 14: load(<syn-4>[1])
> > 15: mat1[1] = @14
> >
> > @11 is removed after splitting, and @4 is subsequently dce'd out. The
> > crucially relevant bit is that we keep both instructions around after
> > splitting them, and we don't actually change the source. This was the
> > simplest approach I could come up with, because it means we can
> > essentially split one instruction at a time, without having to recurse.
>
> Cool. Yeah, that looks simpler than what I was envisioning. I'm not
> sure how turning struct hlsl_ir_node into a union would help, or
> matter, though.

For that kind of splitting, it doesn't. However, it can help:

* splitting constructors, where the type doesn't need to change, and we 
can replace the HLSL_IR_CONSTRUCTOR instruction with HLSL_IR_LOAD;

* constant folding, where we can replace HLSL_IR_EXPR with HLSL_IR_CONSTANT;

* function inlining, where we replace HLSL_IR_CALL with HLSL_IR_LOAD of 
a synthetic return variable.

[PATCH 4/5] d3dcompiler: Store derefs as an offset to a variable.

[Matteo Bruni]

On Wed, May 20, 2020 at 12:21 AM Zebediah Figura
<zfigura(a)codeweavers.com> wrote:
> For that kind of splitting, it doesn't. However, it can help:
>
> * splitting constructors, where the type doesn't need to change, and we
> can replace the HLSL_IR_CONSTRUCTOR instruction with HLSL_IR_LOAD;
>
> * constant folding, where we can replace HLSL_IR_EXPR with HLSL_IR_CONSTANT;
>
> * function inlining, where we replace HLSL_IR_CALL with HLSL_IR_LOAD of
> a synthetic return variable.

Okay, I see. I think it might be acceptable, although I don't feel
like it's going to help too much. With the usual caveats...

I guess I'm saying: I'd go ahead without the union, until it becomes
clear that it would actually help significantly.

[PATCH 5/5] d3dcompiler: Rename HLSL_IR_DEREF to HLSL_IR_LOAD.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/d3dcompiler_private.h |  10 +--
 dlls/d3dcompiler_43/hlsl.y                | 101 +++++++++++-----------
 dlls/d3dcompiler_43/utils.c               |  18 ++--
 3 files changed, 64 insertions(+), 65 deletions(-)

diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 983057d543c..b1dfcbda3d1 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -648,9 +648,9 @@ enum hlsl_ir_node_type
     HLSL_IR_ASSIGNMENT = 0,
     HLSL_IR_CONSTANT,
     HLSL_IR_CONSTRUCTOR,
-    HLSL_IR_DEREF,
     HLSL_IR_EXPR,
     HLSL_IR_IF,
+    HLSL_IR_LOAD,
     HLSL_IR_LOOP,
     HLSL_IR_JUMP,
     HLSL_IR_SWIZZLE,
@@ -848,7 +848,7 @@ struct hlsl_deref
     DWORD writemask;
 };
 
-struct hlsl_ir_deref
+struct hlsl_ir_load
 {
     struct hlsl_ir_node node;
     struct hlsl_deref src;
@@ -1005,10 +1005,10 @@ static inline struct hlsl_ir_expr *expr_from_node(const struct hlsl_ir_node *nod
     return CONTAINING_RECORD(node, struct hlsl_ir_expr, node);
 }
 
-static inline struct hlsl_ir_deref *deref_from_node(const struct hlsl_ir_node *node)
+static inline struct hlsl_ir_load *load_from_node(const struct hlsl_ir_node *node)
 {
-    assert(node->type == HLSL_IR_DEREF);
-    return CONTAINING_RECORD(node, struct hlsl_ir_deref, node);
+    assert(node->type == HLSL_IR_LOAD);
+    return CONTAINING_RECORD(node, struct hlsl_ir_load, node);
 }
 
 static inline struct hlsl_ir_constant *constant_from_node(const struct hlsl_ir_node *node)
diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 24157c8fdc9..9ee4f9e89ba 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -561,32 +561,32 @@ static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct s
     return c;
 }
 
-static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
+static struct hlsl_ir_load *new_var_load(struct hlsl_ir_var *var, const struct source_location loc)
 {
-    struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
+    struct hlsl_ir_load *load = d3dcompiler_alloc(sizeof(*load));
 
-    if (!deref)
+    if (!load)
     {
         ERR("Out of memory.\n");
         return NULL;
     }
-    init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
-    deref->src.var = var;
+    init_node(&load->node, HLSL_IR_LOAD, var->data_type, loc);
+    load->src.var = var;
     if (type_is_single_reg(var->data_type))
-        deref->src.writemask = (1 << var->data_type->dimx) - 1;
-    return deref;
+        load->src.writemask = (1 << var->data_type->dimx) - 1;
+    return load;
 }
 
-static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
+static struct hlsl_ir_load *new_load(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
         struct hlsl_type *data_type, const struct source_location loc)
 {
     struct hlsl_ir_node *add = NULL;
-    struct hlsl_ir_deref *deref;
+    struct hlsl_ir_load *load;
     struct hlsl_ir_var *var;
 
-    if (var_node->type == HLSL_IR_DEREF)
+    if (var_node->type == HLSL_IR_LOAD)
     {
-        const struct hlsl_deref *src = &deref_from_node(var_node)->src;
+        const struct hlsl_deref *src = &load_from_node(var_node)->src;
 
         var = src->var;
         if (src->offset)
@@ -614,18 +614,18 @@ static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hls
         list_add_after(&var_node->entry, &assign->node.entry);
     }
 
-    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
+    if (!(load = d3dcompiler_alloc(sizeof(*load))))
         return NULL;
-    init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
-    deref->src.var = var;
-    deref->src.offset = offset;
+    init_node(&load->node, HLSL_IR_LOAD, data_type, loc);
+    load->src.var = var;
+    load->src.offset = offset;
     if (type_is_single_reg(data_type))
-        deref->src.writemask = (1 << data_type->dimx) - 1;
-    list_add_after(&offset->entry, &deref->node.entry);
-    return deref;
+        load->src.writemask = (1 << data_type->dimx) - 1;
+    list_add_after(&offset->entry, &load->node.entry);
+    return load;
 }
 
-static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
+static struct hlsl_ir_load *new_record_load(struct hlsl_ir_node *record,
         const struct hlsl_struct_field *field, const struct source_location loc)
 {
     struct hlsl_ir_constant *c;
@@ -634,16 +634,16 @@ static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
         return NULL;
     list_add_after(&record->entry, &c->node.entry);
 
-    return new_deref(record, &c->node, field->type, loc);
+    return new_load(record, &c->node, field->type, loc);
 }
 
-static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
+static struct hlsl_ir_load *new_array_load(struct hlsl_ir_node *array,
         struct hlsl_ir_node *index, const struct source_location loc)
 {
     const struct hlsl_type *expr_type = array->data_type;
     struct hlsl_type *data_type;
 
-    TRACE("Array dereference from type %s.\n", debug_hlsl_type(expr_type));
+    TRACE("Array load from type %s.\n", debug_hlsl_type(expr_type));
 
     if (expr_type->type == HLSL_CLASS_ARRAY)
     {
@@ -678,7 +678,7 @@ static struct hlsl_ir_deref *new_array_deref(struct hlsl_ir_node *array,
         index = mul;
     }
 
-    return new_deref(array, index, data_type, loc);
+    return new_load(array, index, data_type, loc);
 }
 
 static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
@@ -687,7 +687,7 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
     struct hlsl_type *type = var->data_type;
     struct hlsl_struct_field *field;
     struct hlsl_ir_node *assignment;
-    struct hlsl_ir_deref *deref;
+    struct hlsl_ir_load *load;
     unsigned int i = 0;
 
     if (initializer_size(initializer) != components_count_type(type))
@@ -711,14 +711,13 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
         }
         if (components_count_type(field->type) == components_count_type(node->data_type))
         {
-            deref = new_record_deref(&new_var_deref(var, var->loc)->node, field, node->loc);
-            if (!deref)
+            if (!(load = new_record_load(&new_var_load(var, var->loc)->node, field, node->loc)))
             {
                 ERR("Out of memory.\n");
                 break;
             }
-            list_add_tail(list, &deref->node.entry);
-            assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, node);
+            list_add_tail(list, &load->node.entry);
+            assignment = make_assignment(&load->node, ASSIGN_OP_ASSIGN, node);
             list_add_tail(list, &assignment->entry);
         }
         else
@@ -809,7 +808,7 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers,
         if (v->initializer.args_count)
         {
             unsigned int size = initializer_size(&v->initializer);
-            struct hlsl_ir_deref *deref;
+            struct hlsl_ir_load *load;
 
             TRACE("Variable with initializer.\n");
             if (type->type <= HLSL_CLASS_LAST_NUMERIC
@@ -865,9 +864,9 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers,
             list_move_tail(statements_list, v->initializer.instrs);
             d3dcompiler_free(v->initializer.instrs);
 
-            deref = new_var_deref(var, var->loc);
-            list_add_tail(statements_list, &deref->node.entry);
-            assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
+            load = new_var_load(var, var->loc);
+            list_add_tail(statements_list, &load->node.entry);
+            assignment = make_assignment(&load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]);
             d3dcompiler_free(v->initializer.args);
             list_add_tail(statements_list, &assignment->entry);
         }
@@ -1225,8 +1224,8 @@ static unsigned int evaluate_array_dimension(struct hlsl_ir_node *node)
         }
     }
     case HLSL_IR_CONSTRUCTOR:
-    case HLSL_IR_DEREF:
     case HLSL_IR_EXPR:
+    case HLSL_IR_LOAD:
     case HLSL_IR_SWIZZLE:
         FIXME("Unhandled type %s.\n", debug_node_type(node->type));
         return 0;
@@ -2252,7 +2251,7 @@ primary_expr:             C_FLOAT
                             }
                         | VAR_IDENTIFIER
                             {
-                                struct hlsl_ir_deref *deref;
+                                struct hlsl_ir_load *load;
                                 struct hlsl_ir_var *var;
 
                                 if (!(var = get_variable(hlsl_ctx.cur_scope, $1)))
@@ -2262,9 +2261,9 @@ primary_expr:             C_FLOAT
                                     set_parse_status(&hlsl_ctx.status, PARSE_ERR);
                                     YYABORT;
                                 }
-                                if ((deref = new_var_deref(var, get_location(&@1))))
+                                if ((load = new_var_load(var, get_location(&@1))))
                                 {
-                                    if (!($$ = make_list(&deref->node)))
+                                    if (!($$ = make_list(&load->node)))
                                         YYABORT;
                                 }
                                 else
@@ -2329,14 +2328,14 @@ postfix_expr:             primary_expr
                                     {
                                         if (!strcmp($3, field->name))
                                         {
-                                            struct hlsl_ir_deref *deref = new_record_deref(node, field, loc);
+                                            struct hlsl_ir_load *load = new_record_load(node, field, loc);
 
-                                            if (!deref)
+                                            if (!load)
                                             {
                                                 ERR("Out of memory\n");
                                                 YYABORT;
                                             }
-                                            $$ = append_unop($1, &deref->node);
+                                            $$ = append_unop($1, &load->node);
                                             break;
                                         }
                                     }
@@ -2369,7 +2368,7 @@ postfix_expr:             primary_expr
                             }
                         | postfix_expr '[' expr ']'
                             {
-                                struct hlsl_ir_deref *deref;
+                                struct hlsl_ir_load *load;
 
                                 if (node_from_list($3)->data_type->type != HLSL_CLASS_SCALAR)
                                 {
@@ -2379,13 +2378,13 @@ postfix_expr:             primary_expr
                                     YYABORT;
                                 }
 
-                                if (!(deref = new_array_deref(node_from_list($1), node_from_list($3), get_location(&@2))))
+                                if (!(load = new_array_load(node_from_list($1), node_from_list($3), get_location(&@2))))
                                 {
                                     free_instr_list($1);
                                     free_instr_list($3);
                                     YYABORT;
                                 }
-                                $$ = append_binop($1, $3, &deref->node);
+                                $$ = append_binop($1, $3, &load->node);
                             }
                           /* "var_modifiers" doesn't make sense in this case, but it's needed
                              in the grammar to avoid shift/reduce conflicts. */
@@ -2818,15 +2817,6 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
                 constructor->args[i]->last_read = instr->index;
             break;
         }
-        case HLSL_IR_DEREF:
-        {
-            struct hlsl_ir_deref *deref = deref_from_node(instr);
-            var = deref->src.var;
-            var->last_read = loop_last ? max(instr->index, loop_last) : instr->index;
-            if (deref->src.offset)
-                deref->src.offset->last_read = instr->index;
-            break;
-        }
         case HLSL_IR_EXPR:
         {
             struct hlsl_ir_expr *expr = expr_from_node(instr);
@@ -2853,6 +2843,15 @@ static void compute_liveness_recurse(struct list *instrs, unsigned int loop_firs
                 jump->return_value->last_read = instr->index;
             break;
         }
+        case HLSL_IR_LOAD:
+        {
+            struct hlsl_ir_load *load = load_from_node(instr);
+            var = load->src.var;
+            var->last_read = loop_last ? max(instr->index, loop_last) : instr->index;
+            if (load->src.offset)
+                load->src.offset->last_read = instr->index;
+            break;
+        }
         case HLSL_IR_LOOP:
         {
             struct hlsl_ir_loop *loop = loop_from_node(instr);
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 4b1f99de65d..440ad491884 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -1450,7 +1450,7 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign
         return NULL;
     }
 
-    while (lhs->type != HLSL_IR_DEREF)
+    while (lhs->type != HLSL_IR_LOAD)
     {
         struct hlsl_ir_node *lhs_inner;
 
@@ -1527,7 +1527,7 @@ struct hlsl_ir_node *make_assignment(struct hlsl_ir_node *lhs, enum parse_assign
 
     rhs = implicit_conversion(rhs, type, &rhs->loc);
 
-    assign->lhs = deref_from_node(lhs)->src;
+    assign->lhs = load_from_node(lhs)->src;
     if (assign_op != ASSIGN_OP_ASSIGN)
     {
         enum hlsl_ir_expr_op op = op_from_assignment(assign_op);
@@ -1791,9 +1791,9 @@ const char *debug_node_type(enum hlsl_ir_node_type type)
         "HLSL_IR_ASSIGNMENT",
         "HLSL_IR_CONSTANT",
         "HLSL_IR_CONSTRUCTOR",
-        "HLSL_IR_DEREF",
         "HLSL_IR_EXPR",
         "HLSL_IR_IF",
+        "HLSL_IR_LOAD",
         "HLSL_IR_LOOP",
         "HLSL_IR_JUMP",
         "HLSL_IR_SWIZZLE",
@@ -2096,8 +2096,8 @@ static void debug_dump_instr(const struct hlsl_ir_node *instr)
         case HLSL_IR_EXPR:
             debug_dump_ir_expr(expr_from_node(instr));
             break;
-        case HLSL_IR_DEREF:
-            debug_dump_deref(&deref_from_node(instr)->src);
+        case HLSL_IR_LOAD:
+            debug_dump_deref(&load_from_node(instr)->src);
             break;
         case HLSL_IR_CONSTANT:
             debug_dump_ir_constant(constant_from_node(instr));
@@ -2195,9 +2195,9 @@ static void free_ir_constant(struct hlsl_ir_constant *constant)
     d3dcompiler_free(constant);
 }
 
-static void free_ir_deref(struct hlsl_ir_deref *deref)
+static void free_ir_load(struct hlsl_ir_load *load)
 {
-    d3dcompiler_free(deref);
+    d3dcompiler_free(load);
 }
 
 static void free_ir_swizzle(struct hlsl_ir_swizzle *swizzle)
@@ -2245,8 +2245,8 @@ void free_instr(struct hlsl_ir_node *node)
         case HLSL_IR_CONSTANT:
             free_ir_constant(constant_from_node(node));
             break;
-        case HLSL_IR_DEREF:
-            free_ir_deref(deref_from_node(node));
+        case HLSL_IR_LOAD:
+            free_ir_load(load_from_node(node));
             break;
         case HLSL_IR_SWIZZLE:
             free_ir_swizzle(swizzle_from_node(node));
-- 
2.26.2

[PATCH 5/5] d3dcompiler: Rename HLSL_IR_DEREF to HLSL_IR_LOAD.

[Matteo Bruni]

On Mon, May 4, 2020 at 10:05 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  dlls/d3dcompiler_43/d3dcompiler_private.h |  10 +--
>  dlls/d3dcompiler_43/hlsl.y                | 101 +++++++++++-----------
>  dlls/d3dcompiler_43/utils.c               |  18 ++--
>  3 files changed, 64 insertions(+), 65 deletions(-)

I don't know how you feel about it but, now that I see the whole
thing, I really like it. Thanks for sticking with me and my ramblings
:)

[Matteo Bruni]

Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com>

[Matteo Bruni]

On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
> There are of course other options to handle this situation, e.g. allowing
> hlsl_deref to contain either hlsl_ir_var or hlsl_ir_node, or introducing another
> deref type (e.g. HLSL_IR_DEREF and HLSL_IR_OFFSET).
>
> I'm inclined to go with this one, mostly because we don't need to handle
> multiple cases when dealing with derefs (or another case when dealing with node
> types).

First of all, sorry for the late review...
> dlls/d3dcompiler_43/hlsl.y | 68 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index db305361738..01b210419c8 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -125,6 +125,11 @@ static void check_invalid_matrix_modifiers(DWORD modifiers, struct source_locati
>      }
>  }
>
> +static BOOL type_is_single_reg(const struct hlsl_type *type)
> +{
> +    return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;

In theory there are more possible cases (e.g. matrix with suitable
dimensions and majority) although I'm not sure you want to handle
those in the same way.
> +}
> +
>  static BOOL declare_variable(struct hlsl_ir_var *decl, BOOL local)
>  {
>      BOOL ret;
> @@ -507,6 +512,41 @@ static struct hlsl_ir_jump *new_return(struct hlsl_ir_node *value, struct source
>      return jump;
>  }
>
> +static struct hlsl_ir_var *new_synthetic_var(const char *name, struct hlsl_type *type,
> +        const struct source_location loc)
> +{
> +    struct hlsl_ir_var *var;
> +
> +    if (!(var = d3dcompiler_alloc(sizeof(*var))))
> +    {
> +        hlsl_ctx.status = PARSE_ERR;
> +        return NULL;
> +    }
> +
> +    var->name = strdup(name);
> +    var->data_type = type;
> +    var->loc = loc;
> +    list_add_tail(&hlsl_ctx.globals->vars, &var->scope_entry);
> +    return var;
> +}
> +
> +static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs)
> +{
> +    struct hlsl_ir_assignment *assign;
> +
> +    if (!(assign = d3dcompiler_alloc(sizeof(*assign))))
> +        return NULL;
> +
> +    init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
> +    assign->lhs.type = HLSL_IR_DEREF_VAR;
> +    assign->lhs.v.var = lhs;
> +    assign->rhs = rhs;
> +    if (type_is_single_reg(lhs->data_type))
> +        assign->writemask = (1 << lhs->data_type->dimx) - 1;
> +
> +    return assign;
> +}
> +
>  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
>  {
>      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> @@ -525,13 +565,33 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
>  static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
>          struct hlsl_struct_field *field, const struct source_location loc)
>  {
> -    struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> +    struct hlsl_ir_deref *deref;
>
> -    if (!deref)
> +    if (record->type != HLSL_IR_DEREF)
>      {
> -        ERR("Out of memory.\n");
> -        return NULL;
> +        struct hlsl_ir_assignment *assign;
> +        struct hlsl_ir_var *var;
> +        char name[27];
> +
> +        sprintf(name, "<deref-%p>", record);
> +        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
> +            return NULL;
> +
> +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
> +
> +        if (!(assign = make_simple_assignment(var, record)))
> +            return NULL;
> +        list_add_after(&record->entry, &assign->node.entry);
> +
> +        if (!(deref = new_var_deref(var, var->loc)))
> +            return NULL;
> +        list_add_after(&assign->node.entry, &deref->node.entry);
> +        record = &deref->node;
>      }
> +
> +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> +        return NULL;
> +
>      init_node(&deref->node, HLSL_IR_DEREF, field->type, loc);
>      deref->src.type = HLSL_IR_DEREF_RECORD;
>      deref->src.v.record.record = record;

This looks fine to me. It would have been nicer if we had a test
exercising this new path already in the test suite... but I don't know
that the parser currently supports any of those.

[Zebediah Figura]

On 5/19/20 2:31 PM, Matteo Bruni wrote:
> On Mon, May 4, 2020 at 10:04 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> > There are of course other options to handle this situation, e.g. allowing
> > hlsl_deref to contain either hlsl_ir_var or hlsl_ir_node, or introducing another
> > deref type (e.g. HLSL_IR_DEREF and HLSL_IR_OFFSET).
> >
> > I'm inclined to go with this one, mostly because we don't need to handle
> > multiple cases when dealing with derefs (or another case when dealing with node
> > types).
>
> First of all, sorry for the late review...
> > dlls/d3dcompiler_43/hlsl.y | 68 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 64 insertions(+), 4 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index db305361738..01b210419c8 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -125,6 +125,11 @@ static void check_invalid_matrix_modifiers(DWORD modifiers, struct source_locati
> >      }
> >  }
> >
> > +static BOOL type_is_single_reg(const struct hlsl_type *type)
> > +{
> > +    return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
>
> In theory there are more possible cases (e.g. matrix with suitable
> dimensions and majority) although I'm not sure you want to handle
> those in the same way.

Yeah, that thought did occur to me. I decided to leave it alone for now,
though making this a separate function does help give us the freedom to
change it later.
> > +}
> > +
> >  static BOOL declare_variable(struct hlsl_ir_var *decl, BOOL local)
> >  {
> >      BOOL ret;
> > @@ -507,6 +512,41 @@ static struct hlsl_ir_jump *new_return(struct hlsl_ir_node *value, struct source
> >      return jump;
> >  }
> >
> > +static struct hlsl_ir_var *new_synthetic_var(const char *name, struct hlsl_type *type,
> > +        const struct source_location loc)
> > +{
> > +    struct hlsl_ir_var *var;
> > +
> > +    if (!(var = d3dcompiler_alloc(sizeof(*var))))
> > +    {
> > +        hlsl_ctx.status = PARSE_ERR;
> > +        return NULL;
> > +    }
> > +
> > +    var->name = strdup(name);
> > +    var->data_type = type;
> > +    var->loc = loc;
> > +    list_add_tail(&hlsl_ctx.globals->vars, &var->scope_entry);
> > +    return var;
> > +}
> > +
> > +static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs)
> > +{
> > +    struct hlsl_ir_assignment *assign;
> > +
> > +    if (!(assign = d3dcompiler_alloc(sizeof(*assign))))
> > +        return NULL;
> > +
> > +    init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
> > +    assign->lhs.type = HLSL_IR_DEREF_VAR;
> > +    assign->lhs.v.var = lhs;
> > +    assign->rhs = rhs;
> > +    if (type_is_single_reg(lhs->data_type))
> > +        assign->writemask = (1 << lhs->data_type->dimx) - 1;
> > +
> > +    return assign;
> > +}
> > +
> >  static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc)
> >  {
> >      struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> > @@ -525,13 +565,33 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct
> >  static struct hlsl_ir_deref *new_record_deref(struct hlsl_ir_node *record,
> >          struct hlsl_struct_field *field, const struct source_location loc)
> >  {
> > -    struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref));
> > +    struct hlsl_ir_deref *deref;
> >
> > -    if (!deref)
> > +    if (record->type != HLSL_IR_DEREF)
> >      {
> > -        ERR("Out of memory.\n");
> > -        return NULL;
> > +        struct hlsl_ir_assignment *assign;
> > +        struct hlsl_ir_var *var;
> > +        char name[27];
> > +
> > +        sprintf(name, "<deref-%p>", record);
> > +        if (!(var = new_synthetic_var(name, record->data_type, record->loc)))
> > +            return NULL;
> > +
> > +        TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(record->type));
> > +
> > +        if (!(assign = make_simple_assignment(var, record)))
> > +            return NULL;
> > +        list_add_after(&record->entry, &assign->node.entry);
> > +
> > +        if (!(deref = new_var_deref(var, var->loc)))
> > +            return NULL;
> > +        list_add_after(&assign->node.entry, &deref->node.entry);
> > +        record = &deref->node;
> >      }
> > +
> > +    if (!(deref = d3dcompiler_alloc(sizeof(*deref))))
> > +        return NULL;
> > +
> >      init_node(&deref->node, HLSL_IR_DEREF, field->type, loc);
> >      deref->src.type = HLSL_IR_DEREF_RECORD;
> >      deref->src.v.record.record = record;
>
> This looks fine to me. It would have been nicer if we had a test
> exercising this new path already in the test suite... but I don't know
> that the parser currently supports any of those.

I think the only way to get this is by returning a struct from a
function, unless there's something I'm missing. But I'll certainly put
it on the list of tests that need adding.

[Matteo Bruni]

On Tue, May 19, 2020 at 9:36 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 5/19/20 2:31 PM, Matteo Bruni wrote:
> > This looks fine to me. It would have been nicer if we had a test
> > exercising this new path already in the test suite... but I don't know
> > that the parser currently supports any of those.
>
> I think the only way to get this is by returning a struct from a
> function, unless there's something I'm missing. But I'll certainly put
> it on the list of tests that need adding.

FWIW I couldn't think of anything else either.