[PATCH 1/5] d3dcompiler: Create the expression in append_binop().

[Zebediah Figura]

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

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 96a27d6a30d..43ffa43a02a 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -1193,12 +1193,14 @@ static struct list *append_unop(struct list *list, struct hlsl_ir_node *node)
     return list;
 }
 
-static struct list *append_binop(struct list *first, struct list *second, struct hlsl_ir_node *node)
+static struct list *add_binary_expr(struct list *list1, struct list *list2,
+        enum hlsl_ir_expr_op op, struct source_location loc)
 {
-    list_move_tail(first, second);
-    d3dcompiler_free(second);
-    list_add_tail(first, &node->entry);
-    return first;
+    struct hlsl_ir_node *arg1 = node_from_list(list1), *arg2 = node_from_list(list2);
+    list_move_tail(list1, list2);
+    d3dcompiler_free(list2);
+    list_add_tail(list1, &new_binary_expr(op, arg1, arg2, loc)->entry);
+    return list1;
 }
 
 static struct list *make_list(struct hlsl_ir_node *node)
@@ -2578,40 +2580,33 @@ unary_op:                 '+'
                                 $$ = UNARY_OP_BITNOT;
                             }
 
-mul_expr:                 unary_expr
-                            {
-                                $$ = $1;
-                            }
-                        | mul_expr '*' unary_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_MUL,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | mul_expr '/' unary_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_DIV,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | mul_expr '%' unary_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_MOD,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
+mul_expr:
 
-add_expr:                 mul_expr
-                            {
-                                $$ = $1;
-                            }
-                        | add_expr '+' mul_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_ADD,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | add_expr '-' mul_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_SUB,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
+      unary_expr
+    | mul_expr '*' unary_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_MUL, get_location(&@2));
+        }
+    | mul_expr '/' unary_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_DIV, get_location(&@2));
+        }
+    | mul_expr '%' unary_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_MOD, get_location(&@2));
+        }
+
+add_expr:
+
+      mul_expr
+    | add_expr '+' mul_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_ADD, get_location(&@2));
+        }
+    | add_expr '-' mul_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_SUB, get_location(&@2));
+        }
 
 shift_expr:               add_expr
                             {
@@ -2626,45 +2621,37 @@ shift_expr:               add_expr
                                 FIXME("Right shift\n");
                             }
 
-relational_expr:          shift_expr
-                            {
-                                $$ = $1;
-                            }
-                        | relational_expr '<' shift_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_LESS,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | relational_expr '>' shift_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_GREATER,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | relational_expr OP_LE shift_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_LEQUAL,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | relational_expr OP_GE shift_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_GEQUAL,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
+relational_expr:
 
-equality_expr:            relational_expr
-                            {
-                                $$ = $1;
-                            }
-                        | equality_expr OP_EQ relational_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_EQUAL,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
-                        | equality_expr OP_NE relational_expr
-                            {
-                                $$ = append_binop($1, $3, new_binary_expr(HLSL_IR_BINOP_NEQUAL,
-                                        node_from_list($1), node_from_list($3), get_location(&@2)));
-                            }
+      shift_expr
+    | relational_expr '<' shift_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_LESS, get_location(&@2));
+        }
+    | relational_expr '>' shift_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_GREATER, get_location(&@2));
+        }
+    | relational_expr OP_LE shift_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_LEQUAL, get_location(&@2));
+        }
+    | relational_expr OP_GE shift_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_GEQUAL, get_location(&@2));
+        }
+
+equality_expr:
+
+      relational_expr
+    | equality_expr OP_EQ relational_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_EQUAL, get_location(&@2));
+        }
+    | equality_expr OP_NE relational_expr
+        {
+            $$ = add_binary_expr($1, $3, HLSL_IR_BINOP_NEQUAL, get_location(&@2));
+        }
 
 bitand_expr:              equality_expr
                             {
-- 
2.27.0

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/d3dcompiler_private.h | 27 ++++++-----------
 dlls/d3dcompiler_43/hlsl.y                | 36 ++++++++++++++++++++---
 dlls/d3dcompiler_43/utils.c               |  5 ++--
 3 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
index 97b84c44945..50457b2e64e 100644
--- a/dlls/d3dcompiler_43/d3dcompiler_private.h
+++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
@@ -1059,9 +1059,18 @@ static inline void init_node(struct hlsl_ir_node *node, enum hlsl_ir_node_type t
 
 struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lhs,
         enum parse_assign_op assign_op, struct hlsl_ir_node *rhs) DECLSPEC_HIDDEN;
+struct hlsl_ir_expr *add_expr(struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[3],
+        struct source_location *loc) DECLSPEC_HIDDEN;
 struct hlsl_ir_node *add_implicit_conversion(struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *type,
         struct source_location *loc) DECLSPEC_HIDDEN;
 
+struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
+        struct source_location *loc) DECLSPEC_HIDDEN;
+struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1,
+        struct hlsl_ir_node *arg2) DECLSPEC_HIDDEN;
+struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg,
+        struct source_location loc) DECLSPEC_HIDDEN;
+
 BOOL add_declaration(struct hlsl_scope *scope, struct hlsl_ir_var *decl, BOOL local_var) DECLSPEC_HIDDEN;
 struct hlsl_ir_var *get_variable(struct hlsl_scope *scope, const char *name) DECLSPEC_HIDDEN;
 void free_declaration(struct hlsl_ir_var *decl) DECLSPEC_HIDDEN;
@@ -1075,10 +1084,6 @@ BOOL find_function(const char *name) DECLSPEC_HIDDEN;
 unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN;
 BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN;
 BOOL compatible_data_types(struct hlsl_type *s1, struct hlsl_type *s2) DECLSPEC_HIDDEN;
-struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **operands,
-        struct source_location *loc) DECLSPEC_HIDDEN;
-struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
-	struct source_location *loc) DECLSPEC_HIDDEN;
 void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
 BOOL pop_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
 void init_functions_tree(struct wine_rb_tree *funcs) DECLSPEC_HIDDEN;
@@ -1098,20 +1103,6 @@ void free_instr(struct hlsl_ir_node *node) DECLSPEC_HIDDEN;
 void free_instr_list(struct list *list) DECLSPEC_HIDDEN;
 void free_function_rb(struct wine_rb_entry *entry, void *context) DECLSPEC_HIDDEN;
 
-static inline struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op,
-        struct hlsl_ir_node *op1, struct source_location loc)
-{
-    struct hlsl_ir_node *operands[3] = {op1};
-    return &new_expr(op, operands, &loc)->node;
-}
-
-static inline struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
-        struct hlsl_ir_node *op1, struct hlsl_ir_node *op2, struct source_location loc)
-{
-    struct hlsl_ir_node *operands[3] = {op1, op2};
-    return &new_expr(op, operands, &loc)->node;
-}
-
 #define MAKE_TAG(ch0, ch1, ch2, ch3) \
     ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \
     ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 ))
diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 43ffa43a02a..4e43a802750 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -608,6 +608,34 @@ static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct s
     return c;
 }
 
+struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg, struct source_location loc)
+{
+    struct hlsl_ir_expr *expr;
+
+    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
+        return NULL;
+    init_node(&expr->node, HLSL_IR_EXPR, arg->data_type, loc);
+    expr->op = op;
+    expr->operands[0] = arg;
+    return &expr->node;
+}
+
+struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
+        struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2)
+{
+    struct hlsl_ir_expr *expr;
+
+    assert(compare_hlsl_types(arg1->data_type, arg2->data_type));
+
+    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
+        return NULL;
+    init_node(&expr->node, HLSL_IR_EXPR, arg1->data_type, arg1->loc);
+    expr->op = op;
+    expr->operands[0] = arg1;
+    expr->operands[1] = arg2;
+    return &expr->node;
+}
+
 static struct hlsl_ir_load *new_var_load(struct hlsl_ir_var *var, const struct source_location loc)
 {
     struct hlsl_ir_load *load = d3dcompiler_alloc(sizeof(*load));
@@ -636,7 +664,7 @@ static struct hlsl_ir_load *add_load(struct list *instrs, struct hlsl_ir_node *v
         var = src->var;
         if (src->offset)
         {
-            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))
+            if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset)))
                 return NULL;
             list_add_tail(instrs, &add->entry);
             offset = add;
@@ -712,7 +740,7 @@ static struct hlsl_ir_load *add_array_load(struct list *instrs, struct hlsl_ir_n
     if (!(c = new_uint_constant(data_type->reg_size * 4, loc)))
         return NULL;
     list_add_tail(instrs, &c->node.entry);
-    if (!(mul = new_binary_expr(HLSL_IR_BINOP_MUL, index, &c->node, loc)))
+    if (!(mul = new_binary_expr(HLSL_IR_BINOP_MUL, index, &c->node)))
         return NULL;
     list_add_tail(instrs, &mul->entry);
     index = mul;
@@ -1196,10 +1224,10 @@ static struct list *append_unop(struct list *list, struct hlsl_ir_node *node)
 static struct list *add_binary_expr(struct list *list1, struct list *list2,
         enum hlsl_ir_expr_op op, struct source_location loc)
 {
-    struct hlsl_ir_node *arg1 = node_from_list(list1), *arg2 = node_from_list(list2);
+    struct hlsl_ir_node *args[3] = {node_from_list(list1), node_from_list(list2)};
     list_move_tail(list1, list2);
     d3dcompiler_free(list2);
-    list_add_tail(list1, &new_binary_expr(op, arg1, arg2, loc)->entry);
+    add_expr(list1, op, args, &loc);
     return list1;
 }
 
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 3f063e1a0ae..aadad176915 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -1328,7 +1328,7 @@ struct hlsl_ir_node *add_implicit_conversion(struct list *instrs, struct hlsl_ir
     return &cast->node;
 }
 
-struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **operands,
+struct hlsl_ir_expr *add_expr(struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[3],
         struct source_location *loc)
 {
     struct hlsl_ir_expr *expr;
@@ -1372,6 +1372,7 @@ struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **ope
     expr->operands[0] = operands[0];
     expr->operands[1] = operands[1];
     expr->operands[2] = operands[2];
+    list_add_tail(instrs, &expr->node.entry);
 
     return expr;
 }
@@ -1519,7 +1520,7 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
         struct hlsl_ir_node *expr;
 
         TRACE("Adding an expression for the compound assignment.\n");
-        expr = new_binary_expr(op, lhs, rhs, lhs->loc);
+        expr = new_binary_expr(op, lhs, rhs);
         list_add_after(&rhs->entry, &expr->entry);
         rhs = expr;
     }
-- 
2.27.0

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=73936

Your paranoid android.


=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
Task: Patch failed to apply

=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
Task: Patch failed to apply

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Matteo Bruni]

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

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Matteo Bruni]

On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  dlls/d3dcompiler_43/d3dcompiler_private.h | 27 ++++++-----------
>  dlls/d3dcompiler_43/hlsl.y                | 36 ++++++++++++++++++++---
>  dlls/d3dcompiler_43/utils.c               |  5 ++--
>  3 files changed, 44 insertions(+), 24 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> index 97b84c44945..50457b2e64e 100644
> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> @@ -1059,9 +1059,18 @@ static inline void init_node(struct hlsl_ir_node *node, enum hlsl_ir_node_type t
>
> struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lhs,
>          enum parse_assign_op assign_op, struct hlsl_ir_node *rhs) DECLSPEC_HIDDEN;
> +struct hlsl_ir_expr *add_expr(struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[3],
> +        struct source_location *loc) DECLSPEC_HIDDEN;
>  struct hlsl_ir_node *add_implicit_conversion(struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *type,
>          struct source_location *loc) DECLSPEC_HIDDEN;
>
> +struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> +        struct source_location *loc) DECLSPEC_HIDDEN;
> +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1,
> +        struct hlsl_ir_node *arg2) DECLSPEC_HIDDEN;
> +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg,
> +        struct source_location loc) DECLSPEC_HIDDEN;
> +
>  BOOL add_declaration(struct hlsl_scope *scope, struct hlsl_ir_var *decl, BOOL local_var) DECLSPEC_HIDDEN;
>  struct hlsl_ir_var *get_variable(struct hlsl_scope *scope, const char *name) DECLSPEC_HIDDEN;
>  void free_declaration(struct hlsl_ir_var *decl) DECLSPEC_HIDDEN;
> @@ -1075,10 +1084,6 @@ BOOL find_function(const char *name) DECLSPEC_HIDDEN;
>  unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN;
>  BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN;
>  BOOL compatible_data_types(struct hlsl_type *s1, struct hlsl_type *s2) DECLSPEC_HIDDEN;
> -struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **operands,
> -        struct source_location *loc) DECLSPEC_HIDDEN;
> -struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> -       struct source_location *loc) DECLSPEC_HIDDEN;
>  void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
>  BOOL pop_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
>  void init_functions_tree(struct wine_rb_tree *funcs) DECLSPEC_HIDDEN;
> @@ -1098,20 +1103,6 @@ void free_instr(struct hlsl_ir_node *node) DECLSPEC_HIDDEN;
>  void free_instr_list(struct list *list) DECLSPEC_HIDDEN;
>  void free_function_rb(struct wine_rb_entry *entry, void *context) DECLSPEC_HIDDEN;
>
> -static inline struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op,
> -        struct hlsl_ir_node *op1, struct source_location loc)
> -{
> -    struct hlsl_ir_node *operands[3] = {op1};
> -    return &new_expr(op, operands, &loc)->node;
> -}
> -
> -static inline struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> -        struct hlsl_ir_node *op1, struct hlsl_ir_node *op2, struct source_location loc)
> -{
> -    struct hlsl_ir_node *operands[3] = {op1, op2};
> -    return &new_expr(op, operands, &loc)->node;
> -}
> -
>  #define MAKE_TAG(ch0, ch1, ch2, ch3) \
>      ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \
>      ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 ))
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index 43ffa43a02a..4e43a802750 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -608,6 +608,34 @@ static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct s
>      return c;
>  }
>
> +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg, struct source_location loc)
> +{
> +    struct hlsl_ir_expr *expr;
> +
> +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> +        return NULL;
> +    init_node(&expr->node, HLSL_IR_EXPR, arg->data_type, loc);
> +    expr->op = op;
> +    expr->operands[0] = arg;
> +    return &expr->node;
> +}
> +
> +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> +        struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2)
> +{
> +    struct hlsl_ir_expr *expr;
> +
> +    assert(compare_hlsl_types(arg1->data_type, arg2->data_type));
> +
> +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> +        return NULL;
> +    init_node(&expr->node, HLSL_IR_EXPR, arg1->data_type, arg1->loc);
> +    expr->op = op;
> +    expr->operands[0] = arg1;
> +    expr->operands[1] = arg2;
> +    return &expr->node;
> +}

So, this patch helps in clarifying add_binary_expr() vs
new_binary_expr() and what each one does, which is good. It's not
clear to me what's next for new_unary_expr() and new_binary_expr()
though. I guess you might want to use them from yet-to-be-written
transformation passes. In that case though, wouldn't utils.c be a
better place (if nothing else to add even more separation from e.g.
add_binary_expr())? Or do you plan to move all of this kind of helper
functions into hlsl.y?

Or maybe you want to go in the complete opposite direction and use
them only from hlsl.y? Currently you could make these functions static
and drop them from the private header entirely.

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Zebediah Figura]

On 6/26/20 10:55 AM, Matteo Bruni wrote:
> On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> >  dlls/d3dcompiler_43/d3dcompiler_private.h | 27 ++++++-----------
> >  dlls/d3dcompiler_43/hlsl.y                | 36 ++++++++++++++++++++---
> >  dlls/d3dcompiler_43/utils.c               |  5 ++--
> >  3 files changed, 44 insertions(+), 24 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > index 97b84c44945..50457b2e64e 100644
> > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > @@ -1059,9 +1059,18 @@ static inline void init_node(struct hlsl_ir_node *node, enum hlsl_ir_node_type t
> >
> > struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lhs,
> >          enum parse_assign_op assign_op, struct hlsl_ir_node *rhs) DECLSPEC_HIDDEN;
> > +struct hlsl_ir_expr *add_expr(struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[3],
> > +        struct source_location *loc) DECLSPEC_HIDDEN;
> >  struct hlsl_ir_node *add_implicit_conversion(struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *type,
> >          struct source_location *loc) DECLSPEC_HIDDEN;
> >
> > +struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> > +        struct source_location *loc) DECLSPEC_HIDDEN;
> > +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1,
> > +        struct hlsl_ir_node *arg2) DECLSPEC_HIDDEN;
> > +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg,
> > +        struct source_location loc) DECLSPEC_HIDDEN;
> > +
> >  BOOL add_declaration(struct hlsl_scope *scope, struct hlsl_ir_var *decl, BOOL local_var) DECLSPEC_HIDDEN;
> >  struct hlsl_ir_var *get_variable(struct hlsl_scope *scope, const char *name) DECLSPEC_HIDDEN;
> >  void free_declaration(struct hlsl_ir_var *decl) DECLSPEC_HIDDEN;
> > @@ -1075,10 +1084,6 @@ BOOL find_function(const char *name) DECLSPEC_HIDDEN;
> >  unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN;
> >  BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN;
> >  BOOL compatible_data_types(struct hlsl_type *s1, struct hlsl_type *s2) DECLSPEC_HIDDEN;
> > -struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **operands,
> > -        struct source_location *loc) DECLSPEC_HIDDEN;
> > -struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> > -       struct source_location *loc) DECLSPEC_HIDDEN;
> >  void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
> >  BOOL pop_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
> >  void init_functions_tree(struct wine_rb_tree *funcs) DECLSPEC_HIDDEN;
> > @@ -1098,20 +1103,6 @@ void free_instr(struct hlsl_ir_node *node) DECLSPEC_HIDDEN;
> >  void free_instr_list(struct list *list) DECLSPEC_HIDDEN;
> >  void free_function_rb(struct wine_rb_entry *entry, void *context) DECLSPEC_HIDDEN;
> >
> > -static inline struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op,
> > -        struct hlsl_ir_node *op1, struct source_location loc)
> > -{
> > -    struct hlsl_ir_node *operands[3] = {op1};
> > -    return &new_expr(op, operands, &loc)->node;
> > -}
> > -
> > -static inline struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> > -        struct hlsl_ir_node *op1, struct hlsl_ir_node *op2, struct source_location loc)
> > -{
> > -    struct hlsl_ir_node *operands[3] = {op1, op2};
> > -    return &new_expr(op, operands, &loc)->node;
> > -}
> > -
> >  #define MAKE_TAG(ch0, ch1, ch2, ch3) \
> >      ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \
> >      ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 ))
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index 43ffa43a02a..4e43a802750 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -608,6 +608,34 @@ static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct s
> >      return c;
> >  }
> >
> > +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg, struct source_location loc)
> > +{
> > +    struct hlsl_ir_expr *expr;
> > +
> > +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> > +        return NULL;
> > +    init_node(&expr->node, HLSL_IR_EXPR, arg->data_type, loc);
> > +    expr->op = op;
> > +    expr->operands[0] = arg;
> > +    return &expr->node;
> > +}
> > +
> > +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> > +        struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2)
> > +{
> > +    struct hlsl_ir_expr *expr;
> > +
> > +    assert(compare_hlsl_types(arg1->data_type, arg2->data_type));
> > +
> > +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> > +        return NULL;
> > +    init_node(&expr->node, HLSL_IR_EXPR, arg1->data_type, arg1->loc);
> > +    expr->op = op;
> > +    expr->operands[0] = arg1;
> > +    expr->operands[1] = arg2;
> > +    return &expr->node;
> > +}
>
> So, this patch helps in clarifying add_binary_expr() vs
> new_binary_expr() and what each one does, which is good. It's not
> clear to me what's next for new_unary_expr() and new_binary_expr()
> though. I guess you might want to use them from yet-to-be-written
> transformation passes. In that case though, wouldn't utils.c be a
> better place (if nothing else to add even more separation from e.g.
> add_binary_expr())? Or do you plan to move all of this kind of helper
> functions into hlsl.y?
>
> Or maybe you want to go in the complete opposite direction and use
> them only from hlsl.y? Currently you could make these functions static
> and drop them from the private header entirely.

Yes, that's correct, I have a transformation pass in my tree that uses
them already.

I had kind of been going in the direction of moving everything related
solely to the HLSL compiler from utils.c to hlsl.y. I assumed that was a
preferred organization, and frankly, it's hard to draw clear lines of
separation for the HLSL compiler anyway.

[PATCH 2/5] d3dcompiler: Add the node to the instruction list in new_expr().

[Matteo Bruni]

On Fri, Jun 26, 2020 at 6:00 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 6/26/20 10:55 AM, Matteo Bruni wrote:
> > On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > ---
> > >  dlls/d3dcompiler_43/d3dcompiler_private.h | 27 ++++++-----------
> > >  dlls/d3dcompiler_43/hlsl.y                | 36 ++++++++++++++++++++---
> > >  dlls/d3dcompiler_43/utils.c               |  5 ++--
> > >  3 files changed, 44 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > index 97b84c44945..50457b2e64e 100644
> > > --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> > > @@ -1059,9 +1059,18 @@ static inline void init_node(struct hlsl_ir_node *node, enum hlsl_ir_node_type t
> > >
> > > struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lhs,
> > >          enum parse_assign_op assign_op, struct hlsl_ir_node *rhs) DECLSPEC_HIDDEN;
> > > +struct hlsl_ir_expr *add_expr(struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[3],
> > > +        struct source_location *loc) DECLSPEC_HIDDEN;
> > >  struct hlsl_ir_node *add_implicit_conversion(struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *type,
> > >          struct source_location *loc) DECLSPEC_HIDDEN;
> > >
> > > +struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> > > +        struct source_location *loc) DECLSPEC_HIDDEN;
> > > +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1,
> > > +        struct hlsl_ir_node *arg2) DECLSPEC_HIDDEN;
> > > +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg,
> > > +        struct source_location loc) DECLSPEC_HIDDEN;
> > > +
> > >  BOOL add_declaration(struct hlsl_scope *scope, struct hlsl_ir_var *decl, BOOL local_var) DECLSPEC_HIDDEN;
> > >  struct hlsl_ir_var *get_variable(struct hlsl_scope *scope, const char *name) DECLSPEC_HIDDEN;
> > >  void free_declaration(struct hlsl_ir_var *decl) DECLSPEC_HIDDEN;
> > > @@ -1075,10 +1084,6 @@ BOOL find_function(const char *name) DECLSPEC_HIDDEN;
> > >  unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN;
> > >  BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN;
> > >  BOOL compatible_data_types(struct hlsl_type *s1, struct hlsl_type *s2) DECLSPEC_HIDDEN;
> > > -struct hlsl_ir_expr *new_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node **operands,
> > > -        struct source_location *loc) DECLSPEC_HIDDEN;
> > > -struct hlsl_ir_expr *new_cast(struct hlsl_ir_node *node, struct hlsl_type *type,
> > > -       struct source_location *loc) DECLSPEC_HIDDEN;
> > >  void push_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
> > >  BOOL pop_scope(struct hlsl_parse_ctx *ctx) DECLSPEC_HIDDEN;
> > >  void init_functions_tree(struct wine_rb_tree *funcs) DECLSPEC_HIDDEN;
> > > @@ -1098,20 +1103,6 @@ void free_instr(struct hlsl_ir_node *node) DECLSPEC_HIDDEN;
> > >  void free_instr_list(struct list *list) DECLSPEC_HIDDEN;
> > >  void free_function_rb(struct wine_rb_entry *entry, void *context) DECLSPEC_HIDDEN;
> > >
> > > -static inline struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op,
> > > -        struct hlsl_ir_node *op1, struct source_location loc)
> > > -{
> > > -    struct hlsl_ir_node *operands[3] = {op1};
> > > -    return &new_expr(op, operands, &loc)->node;
> > > -}
> > > -
> > > -static inline struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> > > -        struct hlsl_ir_node *op1, struct hlsl_ir_node *op2, struct source_location loc)
> > > -{
> > > -    struct hlsl_ir_node *operands[3] = {op1, op2};
> > > -    return &new_expr(op, operands, &loc)->node;
> > > -}
> > > -
> > >  #define MAKE_TAG(ch0, ch1, ch2, ch3) \
> > >      ((DWORD)(ch0) | ((DWORD)(ch1) << 8) | \
> > >      ((DWORD)(ch2) << 16) | ((DWORD)(ch3) << 24 ))
> > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > index 43ffa43a02a..4e43a802750 100644
> > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > @@ -608,6 +608,34 @@ static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct s
> > >      return c;
> > >  }
> > >
> > > +struct hlsl_ir_node *new_unary_expr(enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg, struct source_location loc)
> > > +{
> > > +    struct hlsl_ir_expr *expr;
> > > +
> > > +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> > > +        return NULL;
> > > +    init_node(&expr->node, HLSL_IR_EXPR, arg->data_type, loc);
> > > +    expr->op = op;
> > > +    expr->operands[0] = arg;
> > > +    return &expr->node;
> > > +}
> > > +
> > > +struct hlsl_ir_node *new_binary_expr(enum hlsl_ir_expr_op op,
> > > +        struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2)
> > > +{
> > > +    struct hlsl_ir_expr *expr;
> > > +
> > > +    assert(compare_hlsl_types(arg1->data_type, arg2->data_type));
> > > +
> > > +    if (!(expr = d3dcompiler_alloc(sizeof(*expr))))
> > > +        return NULL;
> > > +    init_node(&expr->node, HLSL_IR_EXPR, arg1->data_type, arg1->loc);
> > > +    expr->op = op;
> > > +    expr->operands[0] = arg1;
> > > +    expr->operands[1] = arg2;
> > > +    return &expr->node;
> > > +}
> >
> > So, this patch helps in clarifying add_binary_expr() vs
> > new_binary_expr() and what each one does, which is good. It's not
> > clear to me what's next for new_unary_expr() and new_binary_expr()
> > though. I guess you might want to use them from yet-to-be-written
> > transformation passes. In that case though, wouldn't utils.c be a
> > better place (if nothing else to add even more separation from e.g.
> > add_binary_expr())? Or do you plan to move all of this kind of helper
> > functions into hlsl.y?
> >
> > Or maybe you want to go in the complete opposite direction and use
> > them only from hlsl.y? Currently you could make these functions static
> > and drop them from the private header entirely.
>
> Yes, that's correct, I have a transformation pass in my tree that uses
> them already.
>
> I had kind of been going in the direction of moving everything related
> solely to the HLSL compiler from utils.c to hlsl.y. I assumed that was a
> preferred organization, and frankly, it's hard to draw clear lines of
> separation for the HLSL compiler anyway.

Eh, whatever works, as long as it is consistent.

Originally I planned to put into hlsl.y only the things that have to
do with HLSL parsing proper, though. Relatedly, I see no problems with
having compiler-only stuff in utils.c. Even (gasp) adding one or two
new .c files for other parts of the compiler doesn't seem like the
worst thing one could do.

[PATCH 3/5] d3dcompiler: Rename make_simple_assignment() to new_simple_assignment().

[Zebediah Figura]

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

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 4e43a802750..21828337939 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -557,7 +557,7 @@ static struct hlsl_ir_assignment *new_assignment(struct hlsl_ir_var *var, struct
     return assign;
 }
 
-static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs)
+static struct hlsl_ir_assignment *new_simple_assignment(struct hlsl_ir_var *lhs, struct hlsl_ir_node *rhs)
 {
     return new_assignment(lhs, NULL, rhs, 0, rhs->loc);
 }
@@ -575,7 +575,7 @@ static struct hlsl_ir_jump *add_return(struct list *instrs,
         if (!(return_value = add_implicit_conversion(instrs, return_value, return_type, &loc)))
             return NULL;
 
-        if (!(assignment = make_simple_assignment(hlsl_ctx.cur_function->return_var, return_value)))
+        if (!(assignment = new_simple_assignment(hlsl_ctx.cur_function->return_var, return_value)))
             return NULL;
         list_add_after(&return_value->entry, &assignment->node.entry);
     }
@@ -681,7 +681,7 @@ static struct hlsl_ir_load *add_load(struct list *instrs, struct hlsl_ir_node *v
 
         TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
 
-        if (!(assign = make_simple_assignment(var, var_node)))
+        if (!(assign = new_simple_assignment(var, var_node)))
             return NULL;
 
         list_add_tail(instrs, &assign->node.entry);
-- 
2.27.0

[PATCH 3/5] d3dcompiler: Rename make_simple_assignment() to new_simple_assignment().

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=73938

Your paranoid android.


=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
Task: Patch failed to apply

=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
Task: Patch failed to apply

[PATCH 3/5] d3dcompiler: Rename make_simple_assignment() to new_simple_assignment().

[Matteo Bruni]

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

[PATCH 4/5] d3dcompiler: Print the data type when dumping IR.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/utils.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index aadad176915..30aa9de1dd4 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -1806,9 +1806,11 @@ static void debug_dump_ir_var(const struct hlsl_ir_var *var)
 
 static void debug_dump_deref(const struct hlsl_deref *deref)
 {
-    wine_dbg_printf("deref(");
-    debug_dump_ir_var(deref->var);
-    wine_dbg_printf(")");
+    if (deref->offset)
+        /* Print the variable's type for convenience. */
+        wine_dbg_printf("(%s %s)", debug_hlsl_type(deref->var->data_type), deref->var->name);
+    else
+        wine_dbg_printf("%s", deref->var->name);
     if (deref->offset)
     {
         wine_dbg_printf("[");
@@ -2045,6 +2047,9 @@ static void debug_dump_instr(const struct hlsl_ir_node *instr)
         wine_dbg_printf("%4u: ", instr->index);
     else
         wine_dbg_printf("%p: ", instr);
+
+    wine_dbg_printf("%10s | ", instr->data_type ? debug_hlsl_type(instr->data_type) : "");
+
     switch (instr->type)
     {
         case HLSL_IR_EXPR:
-- 
2.27.0

[PATCH 4/5] d3dcompiler: Print the data type when dumping IR.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=73939

Your paranoid android.


=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
Task: Patch failed to apply

=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
Task: Patch failed to apply

[PATCH 4/5] d3dcompiler: Print the data type when dumping IR.

[Matteo Bruni]

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

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
 dlls/d3dcompiler_43/utils.c | 11 ++++++++---
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
index 21828337939..27aab195b25 100644
--- a/dlls/d3dcompiler_43/hlsl.y
+++ b/dlls/d3dcompiler_43/hlsl.y
@@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
         struct hlsl_ir_node *val, struct source_location *loc)
 {
     struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
+    struct hlsl_type *data_type;
 
     if (!swizzle)
         return NULL;
-    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
-            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
+
+    if (components == 1)
+        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
+    else
+        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
+
+    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
     swizzle->val = val;
     swizzle->swizzle = s;
     return swizzle;
@@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
             for (i = 0; i < $4.args_count; ++i)
             {
                 struct hlsl_ir_node *arg = $4.args[i];
+                struct hlsl_type *data_type;
                 unsigned int width;
 
                 if (arg->data_type->type == HLSL_CLASS_OBJECT)
@@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
                     continue;
                 }
 
-                if (!(arg = add_implicit_conversion($4.instrs, arg,
-                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
+                if (width == 1)
+                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
+                else
+                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
+
+                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
                     continue;
 
                 if (!(assignment = new_assignment(var, NULL, arg,
diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
index 30aa9de1dd4..4f0556103ab 100644
--- a/dlls/d3dcompiler_43/utils.c
+++ b/dlls/d3dcompiler_43/utils.c
@@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
         }
     }
 
-    if (type == HLSL_CLASS_SCALAR)
+    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
         return hlsl_ctx.builtin_types.scalar[base];
     if (type == HLSL_CLASS_VECTOR)
         return hlsl_ctx.builtin_types.vector[base][dimx - 1];
@@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
                 d3dcompiler_free(assign);
                 return NULL;
             }
-            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
+            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
             if (swizzle_type->dimx != width)
-                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
+            {
+                if (width == 1)
+                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
+                else
+                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
+            }
             rhs = &swizzle->node;
         }
         else
-- 
2.27.0

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=73940

Your paranoid android.


=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
error: patch failed: dlls/d3dcompiler_43/hlsl.y:2504
Task: Patch failed to apply

=== debiant (build log) ===

error: patch failed: dlls/d3dcompiler_43/d3dcompiler_private.h:1059
error: patch failed: dlls/d3dcompiler_43/hlsl.y:636
error: patch failed: dlls/d3dcompiler_43/hlsl.y:575
error: patch failed: dlls/d3dcompiler_43/hlsl.y:2504
Task: Patch failed to apply

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Matteo Bruni]

On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
>  dlls/d3dcompiler_43/utils.c | 11 ++++++++---
>  2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index 21828337939..27aab195b25 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
>          struct hlsl_ir_node *val, struct source_location *loc)
>  {
>      struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
> +    struct hlsl_type *data_type;
>
> if (!swizzle)
>          return NULL;
> -    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
> -            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
> +
> +    if (components == 1)
> +        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
> +    else
> +        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
> +
> +    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
>      swizzle->val = val;
>      swizzle->swizzle = s;
>      return swizzle;
> @@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
>              for (i = 0; i < $4.args_count; ++i)
>              {
>                  struct hlsl_ir_node *arg = $4.args[i];
> +                struct hlsl_type *data_type;
>                  unsigned int width;
>
> if (arg->data_type->type == HLSL_CLASS_OBJECT)
> @@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
>                      continue;
>                  }
>
> -                if (!(arg = add_implicit_conversion($4.instrs, arg,
> -                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
> +                if (width == 1)
> +                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
> +                else
> +                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
> +
> +                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
>                      continue;
>
> if (!(assignment = new_assignment(var, NULL, arg,
> diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> index 30aa9de1dd4..4f0556103ab 100644
> --- a/dlls/d3dcompiler_43/utils.c
> +++ b/dlls/d3dcompiler_43/utils.c
> @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
>          }
>      }
>
> -    if (type == HLSL_CLASS_SCALAR)
> +    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
>          return hlsl_ctx.builtin_types.scalar[base];
>      if (type == HLSL_CLASS_VECTOR)
>          return hlsl_ctx.builtin_types.vector[base][dimx - 1];
> @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
>                  d3dcompiler_free(assign);
>                  return NULL;
>              }
> -            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
> +            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
>              if (swizzle_type->dimx != width)
> -                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> +            {
> +                if (width == 1)
> +                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
> +                else
> +                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> +            }
>              rhs = &swizzle->node;
>          }
>          else

What do we gain with this?

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Zebediah Figura]

On 6/26/20 10:56 AM, Matteo Bruni wrote:
> On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> >  dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
> >  dlls/d3dcompiler_43/utils.c | 11 ++++++++---
> >  2 files changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > index 21828337939..27aab195b25 100644
> > --- a/dlls/d3dcompiler_43/hlsl.y
> > +++ b/dlls/d3dcompiler_43/hlsl.y
> > @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
> >          struct hlsl_ir_node *val, struct source_location *loc)
> >  {
> >      struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
> > +    struct hlsl_type *data_type;
> >
> > if (!swizzle)
> >          return NULL;
> > -    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
> > -            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
> > +
> > +    if (components == 1)
> > +        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
> > +    else
> > +        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
> > +
> > +    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
> >      swizzle->val = val;
> >      swizzle->swizzle = s;
> >      return swizzle;
> > @@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
> >              for (i = 0; i < $4.args_count; ++i)
> >              {
> >                  struct hlsl_ir_node *arg = $4.args[i];
> > +                struct hlsl_type *data_type;
> >                  unsigned int width;
> >
> > if (arg->data_type->type == HLSL_CLASS_OBJECT)
> > @@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
> >                      continue;
> >                  }
> >
> > -                if (!(arg = add_implicit_conversion($4.instrs, arg,
> > -                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
> > +                if (width == 1)
> > +                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
> > +                else
> > +                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
> > +
> > +                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
> >                      continue;
> >
> > if (!(assignment = new_assignment(var, NULL, arg,
> > diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> > index 30aa9de1dd4..4f0556103ab 100644
> > --- a/dlls/d3dcompiler_43/utils.c
> > +++ b/dlls/d3dcompiler_43/utils.c
> > @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
> >          }
> >      }
> >
> > -    if (type == HLSL_CLASS_SCALAR)
> > +    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
> >          return hlsl_ctx.builtin_types.scalar[base];
> >      if (type == HLSL_CLASS_VECTOR)
> >          return hlsl_ctx.builtin_types.vector[base][dimx - 1];
> > @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> >                  d3dcompiler_free(assign);
> >                  return NULL;
> >              }
> > -            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
> > +            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
> >              if (swizzle_type->dimx != width)
> > -                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > +            {
> > +                if (width == 1)
> > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
> > +                else
> > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > +            }
> >              rhs = &swizzle->node;
> >          }
> >          else
>
> What do we gain with this?

Not having to deal with vec1 at codegen time, basically, or along
similar lines not having superfluous instructions to cast it to scalar.
Granted, it still exists, but you'd have to use it intentionally...

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Matteo Bruni]

On Fri, Jun 26, 2020 at 6:02 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 6/26/20 10:56 AM, Matteo Bruni wrote:
> > On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > ---
> > >  dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
> > >  dlls/d3dcompiler_43/utils.c | 11 ++++++++---
> > >  2 files changed, 23 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > index 21828337939..27aab195b25 100644
> > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
> > >          struct hlsl_ir_node *val, struct source_location *loc)
> > >  {
> > >      struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
> > > +    struct hlsl_type *data_type;
> > >
> > > if (!swizzle)
> > >          return NULL;
> > > -    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
> > > -            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
> > > +
> > > +    if (components == 1)
> > > +        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
> > > +    else
> > > +        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
> > > +
> > > +    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
> > >      swizzle->val = val;
> > >      swizzle->swizzle = s;
> > >      return swizzle;
> > > @@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
> > >              for (i = 0; i < $4.args_count; ++i)
> > >              {
> > >                  struct hlsl_ir_node *arg = $4.args[i];
> > > +                struct hlsl_type *data_type;
> > >                  unsigned int width;
> > >
> > > if (arg->data_type->type == HLSL_CLASS_OBJECT)
> > > @@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
> > >                      continue;
> > >                  }
> > >
> > > -                if (!(arg = add_implicit_conversion($4.instrs, arg,
> > > -                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
> > > +                if (width == 1)
> > > +                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
> > > +                else
> > > +                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
> > > +
> > > +                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
> > >                      continue;
> > >
> > > if (!(assignment = new_assignment(var, NULL, arg,
> > > diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> > > index 30aa9de1dd4..4f0556103ab 100644
> > > --- a/dlls/d3dcompiler_43/utils.c
> > > +++ b/dlls/d3dcompiler_43/utils.c
> > > @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
> > >          }
> > >      }
> > >
> > > -    if (type == HLSL_CLASS_SCALAR)
> > > +    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
> > >          return hlsl_ctx.builtin_types.scalar[base];
> > >      if (type == HLSL_CLASS_VECTOR)
> > >          return hlsl_ctx.builtin_types.vector[base][dimx - 1];
> > > @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> > >                  d3dcompiler_free(assign);
> > >                  return NULL;
> > >              }
> > > -            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
> > > +            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
> > >              if (swizzle_type->dimx != width)
> > > -                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > +            {
> > > +                if (width == 1)
> > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
> > > +                else
> > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > +            }
> > >              rhs = &swizzle->node;
> > >          }
> > >          else
> >
> > What do we gain with this?
>
> Not having to deal with vec1 at codegen time, basically, or along
> similar lines not having superfluous instructions to cast it to scalar.
> Granted, it still exists, but you'd have to use it intentionally...

My main objection is that, IIRC, we need to keep the distinction
anyway for uniform variables (e.g. I guess it's visible in the
constant table). At that point we could just keep it like that all the
way through the compiler.
WRT casts, they can be avoided by figuring out at "codegen" that they
are actually the same type. Again, we probably need something along
those lines anyway since, especially in SM1, we have to map pretty
much everything to float eventually.

Notice that I can still be convinced that this patch goes in the right
direction...

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Zebediah Figura]

On 6/26/20 11:28 AM, Matteo Bruni wrote:
> On Fri, Jun 26, 2020 at 6:02 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> > On 6/26/20 10:56 AM, Matteo Bruni wrote:
> > > On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > > ---
> > > >  dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
> > > >  dlls/d3dcompiler_43/utils.c | 11 ++++++++---
> > > >  2 files changed, 23 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > > index 21828337939..27aab195b25 100644
> > > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > > @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
> > > >          struct hlsl_ir_node *val, struct source_location *loc)
> > > >  {
> > > >      struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
> > > > +    struct hlsl_type *data_type;
> > > >
> > > > if (!swizzle)
> > > >          return NULL;
> > > > -    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
> > > > -            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
> > > > +
> > > > +    if (components == 1)
> > > > +        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
> > > > +    else
> > > > +        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
> > > > +
> > > > +    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
> > > >      swizzle->val = val;
> > > >      swizzle->swizzle = s;
> > > >      return swizzle;
> > > > @@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
> > > >              for (i = 0; i < $4.args_count; ++i)
> > > >              {
> > > >                  struct hlsl_ir_node *arg = $4.args[i];
> > > > +                struct hlsl_type *data_type;
> > > >                  unsigned int width;
> > > >
> > > > if (arg->data_type->type == HLSL_CLASS_OBJECT)
> > > > @@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
> > > >                      continue;
> > > >                  }
> > > >
> > > > -                if (!(arg = add_implicit_conversion($4.instrs, arg,
> > > > -                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
> > > > +                if (width == 1)
> > > > +                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
> > > > +                else
> > > > +                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
> > > > +
> > > > +                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
> > > >                      continue;
> > > >
> > > > if (!(assignment = new_assignment(var, NULL, arg,
> > > > diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> > > > index 30aa9de1dd4..4f0556103ab 100644
> > > > --- a/dlls/d3dcompiler_43/utils.c
> > > > +++ b/dlls/d3dcompiler_43/utils.c
> > > > @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
> > > >          }
> > > >      }
> > > >
> > > > -    if (type == HLSL_CLASS_SCALAR)
> > > > +    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
> > > >          return hlsl_ctx.builtin_types.scalar[base];
> > > >      if (type == HLSL_CLASS_VECTOR)
> > > >          return hlsl_ctx.builtin_types.vector[base][dimx - 1];
> > > > @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> > > >                  d3dcompiler_free(assign);
> > > >                  return NULL;
> > > >              }
> > > > -            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
> > > > +            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
> > > >              if (swizzle_type->dimx != width)
> > > > -                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > > +            {
> > > > +                if (width == 1)
> > > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
> > > > +                else
> > > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > > +            }
> > > >              rhs = &swizzle->node;
> > > >          }
> > > >          else
> > >
> > > What do we gain with this?
> >
> > Not having to deal with vec1 at codegen time, basically, or along
> > similar lines not having superfluous instructions to cast it to scalar.
> > Granted, it still exists, but you'd have to use it intentionally...
>
> My main objection is that, IIRC, we need to keep the distinction
> anyway for uniform variables (e.g. I guess it's visible in the
> constant table). At that point we could just keep it like that all the
> way through the compiler.
> WRT casts, they can be avoided by figuring out at "codegen" that they
> are actually the same type. Again, we probably need something along
> those lines anyway since, especially in SM1, we have to map pretty
> much everything to float eventually.
>
> Notice that I can still be convinced that this patch goes in the right
> direction...

It gets a little tricky, I think. Uniforms are one case and pretty
clear-cut, i.e. you declare something as "float" or "float1".

Arbitrary expressions are another. The language *does* seem to care
about these in at least one way I could find: you can index a vector
using [0], but not a scalar. According to this criterion, both swizzles
and expression argument coercion count as scalars rather than vectors.
I.e. the code

uniform float1 v;
return v.x[0];

and

uniform float1 v;
uniform float s;
return (v + s)[0];

will both throw an error.

The third case in the patch above, of course, can't be tested, as it
only has internal effect.

Certainly I agree that nothing after parse time should care about the
difference between float and float1, though. I guess we want to reduce
any such generated casts using a copy-prop pass.

[PATCH 5/5] d3dcompiler: Avoid using 1-dimensional vectors as expression types.

[Matteo Bruni]

On Fri, Jun 26, 2020 at 7:14 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> On 6/26/20 11:28 AM, Matteo Bruni wrote:
> > On Fri, Jun 26, 2020 at 6:02 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> > > On 6/26/20 10:56 AM, Matteo Bruni wrote:
> > > > On Tue, Jun 23, 2020 at 12:48 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
> > > > > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > > > > ---
> > > > >  dlls/d3dcompiler_43/hlsl.y  | 19 +++++++++++++++----
> > > > >  dlls/d3dcompiler_43/utils.c | 11 ++++++++---
> > > > >  2 files changed, 23 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> > > > > index 21828337939..27aab195b25 100644
> > > > > --- a/dlls/d3dcompiler_43/hlsl.y
> > > > > +++ b/dlls/d3dcompiler_43/hlsl.y
> > > > > @@ -416,11 +416,17 @@ static struct hlsl_ir_swizzle *new_swizzle(DWORD s, unsigned int components,
> > > > >          struct hlsl_ir_node *val, struct source_location *loc)
> > > > >  {
> > > > >      struct hlsl_ir_swizzle *swizzle = d3dcompiler_alloc(sizeof(*swizzle));
> > > > > +    struct hlsl_type *data_type;
> > > > >
> > > > > if (!swizzle)
> > > > >          return NULL;
> > > > > -    init_node(&swizzle->node, HLSL_IR_SWIZZLE,
> > > > > -            new_hlsl_type(NULL, HLSL_CLASS_VECTOR, val->data_type->base_type, components, 1), *loc);
> > > > > +
> > > > > +    if (components == 1)
> > > > > +        data_type = hlsl_ctx.builtin_types.scalar[val->data_type->base_type];
> > > > > +    else
> > > > > +        data_type = hlsl_ctx.builtin_types.vector[val->data_type->base_type][components - 1];
> > > > > +
> > > > > +    init_node(&swizzle->node, HLSL_IR_SWIZZLE, data_type, *loc);
> > > > >      swizzle->val = val;
> > > > >      swizzle->swizzle = s;
> > > > >      return swizzle;
> > > > > @@ -2488,6 +2494,7 @@ postfix_expr:             primary_expr
> > > > >              for (i = 0; i < $4.args_count; ++i)
> > > > >              {
> > > > >                  struct hlsl_ir_node *arg = $4.args[i];
> > > > > +                struct hlsl_type *data_type;
> > > > >                  unsigned int width;
> > > > >
> > > > > if (arg->data_type->type == HLSL_CLASS_OBJECT)
> > > > > @@ -2504,8 +2511,12 @@ postfix_expr:             primary_expr
> > > > >                      continue;
> > > > >                  }
> > > > >
> > > > > -                if (!(arg = add_implicit_conversion($4.instrs, arg,
> > > > > -                        hlsl_ctx.builtin_types.vector[$2->base_type][width - 1], &arg->loc)))
> > > > > +                if (width == 1)
> > > > > +                    data_type = hlsl_ctx.builtin_types.scalar[$2->base_type];
> > > > > +                else
> > > > > +                    data_type = hlsl_ctx.builtin_types.vector[$2->base_type][width - 1];
> > > > > +
> > > > > +                if (!(arg = add_implicit_conversion($4.instrs, arg, data_type, &arg->loc)))
> > > > >                      continue;
> > > > >
> > > > > if (!(assignment = new_assignment(var, NULL, arg,
> > > > > diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> > > > > index 30aa9de1dd4..4f0556103ab 100644
> > > > > --- a/dlls/d3dcompiler_43/utils.c
> > > > > +++ b/dlls/d3dcompiler_43/utils.c
> > > > > @@ -1294,7 +1294,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
> > > > >          }
> > > > >      }
> > > > >
> > > > > -    if (type == HLSL_CLASS_SCALAR)
> > > > > +    if (type == HLSL_CLASS_SCALAR || (type == HLSL_CLASS_VECTOR && dimx == 1))
> > > > >          return hlsl_ctx.builtin_types.scalar[base];
> > > > >      if (type == HLSL_CLASS_VECTOR)
> > > > >          return hlsl_ctx.builtin_types.vector[base][dimx - 1];
> > > > > @@ -1496,9 +1496,14 @@ struct hlsl_ir_node *add_assignment(struct list *instrs, struct hlsl_ir_node *lh
> > > > >                  d3dcompiler_free(assign);
> > > > >                  return NULL;
> > > > >              }
> > > > > -            assert(swizzle_type->type == HLSL_CLASS_VECTOR);
> > > > > +            assert(swizzle_type->type == HLSL_CLASS_VECTOR || swizzle_type->type == HLSL_CLASS_SCALAR);
> > > > >              if (swizzle_type->dimx != width)
> > > > > -                swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > > > +            {
> > > > > +                if (width == 1)
> > > > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.scalar[swizzle_type->base_type];
> > > > > +                else
> > > > > +                    swizzle->node.data_type = hlsl_ctx.builtin_types.vector[swizzle_type->base_type][width - 1];
> > > > > +            }
> > > > >              rhs = &swizzle->node;
> > > > >          }
> > > > >          else
> > > >
> > > > What do we gain with this?
> > >
> > > Not having to deal with vec1 at codegen time, basically, or along
> > > similar lines not having superfluous instructions to cast it to scalar.
> > > Granted, it still exists, but you'd have to use it intentionally...
> >
> > My main objection is that, IIRC, we need to keep the distinction
> > anyway for uniform variables (e.g. I guess it's visible in the
> > constant table). At that point we could just keep it like that all the
> > way through the compiler.
> > WRT casts, they can be avoided by figuring out at "codegen" that they
> > are actually the same type. Again, we probably need something along
> > those lines anyway since, especially in SM1, we have to map pretty
> > much everything to float eventually.
> >
> > Notice that I can still be convinced that this patch goes in the right
> > direction...
>
> It gets a little tricky, I think. Uniforms are one case and pretty
> clear-cut, i.e. you declare something as "float" or "float1".
>
> Arbitrary expressions are another. The language *does* seem to care
> about these in at least one way I could find: you can index a vector
> using [0], but not a scalar. According to this criterion, both swizzles
> and expression argument coercion count as scalars rather than vectors.
> I.e. the code
>
> uniform float1 v;
> return v.x[0];
>
> and
>
> uniform float1 v;
> uniform float s;
> return (v + s)[0];
>
> will both throw an error.

Oh, that's interesting. So the relevant part of this patch is
technically required for correctness.
> The third case in the patch above, of course, can't be tested, as it
> only has internal effect.
>
> Certainly I agree that nothing after parse time should care about the
> difference between float and float1, though. I guess we want to reduce
> any such generated casts using a copy-prop pass.

Right. My feeling is that such a pass simplifying redundant casts
effectively covers all other cases but I certainly haven't thought it
through.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=73935

Your paranoid android.


=== debiant (build log) ===

/home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_34/../../../wine/dlls/d3dcompiler_34/../d3dcompiler_43/hlsl.y:2720: undefined reference to `append_binop'
/usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_34/../../../wine/dlls/d3dcompiler_34/../d3dcompiler_43/hlsl.y:2414: undefined reference to `append_binop'
/home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_33/../../../wine/dlls/d3dcompiler_33/../d3dcompiler_43/hlsl.y:2720: undefined reference to `append_binop'
/usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_33/../../../wine/dlls/d3dcompiler_33/../d3dcompiler_43/hlsl.y:2414: undefined reference to `append_binop'
collect2: error: ld returned 1 exit status
collect2: error: ld returned 1 exit status
/home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_35/../../../wine/dlls/d3dcompiler_35/../d3dcompiler_43/hlsl.y:2720: undefined reference to `append_binop'
/usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_35/../../../wine/dlls/d3dcompiler_35/../d3dcompiler_43/hlsl.y:2414: undefined reference to `append_binop'
collect2: error: ld returned 1 exit status
/home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_36/../../../wine/dlls/d3dcompiler_36/../d3dcompiler_43/hlsl.y:2720: undefined reference to `append_binop'
/usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-win32/dlls/d3dcompiler_36/../../../wine/dlls/d3dcompiler_36/../d3dcompiler_43/hlsl.y:2414: undefined reference to `append_binop'
collect2: error: ld returned 1 exit status
Task: The win32 Wine build failed

=== debiant (build log) ===

2720: undefined reference to `/home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_35/../../../wine/dlls/d3dcompiler_35/../d3dcompiler_43/hlsl.y:append_binop2720/home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_34/../../../wine/dlls/d3dcompiler_34/../d3dcompiler_43/hlsl.y:'
: undefined reference to `2720append_binop: undefined reference to `'
/usr/bin/x86_64-w64-mingw32-ld: /usr/bin/x86_64-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_33/../../../wine/dlls/d3dcompiler_33/../d3dcompiler_43/hlsl.y:/usr/bin/x86_64-w64-mingw32-ld2414/home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_35/../../../wine/dlls/d3dcompiler_35/../d3dcompiler_43/hlsl.y:: : undefined reference to `2414append_binop: undefined reference to `'
: undefined reference to `append_binop'
/usr/bin/x86_64-w64-mingw32-ld: collect2: error: ld returned 1 exit status
collect2: error: ld returned 1 exit status
collect2: error: ld returned 1 exit status
/home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_36/../../../wine/dlls/d3dcompiler_36/../d3dcompiler_43/hlsl.y:2720: undefined reference to `append_binop'
/usr/bin/x86_64-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-wow64/dlls/d3dcompiler_36/../../../wine/dlls/d3dcompiler_36/../d3dcompiler_43/hlsl.y:2414: undefined reference to `append_binop'
collect2: error: ld returned 1 exit status
Task: The wow64 Wine build failed

[Matteo Bruni]

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