[PATCH vkd3d 1/6] vkd3d-shader/hlsl: Introduce a helper for getting numeric types.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.c |  4 ++--
 libs/vkd3d-shader/hlsl.h | 28 ++++++++++++++++++++++++++++
 libs/vkd3d-shader/hlsl.y | 28 ++++++++++++----------------
 3 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 9bce6beb..421aaf27 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -560,7 +560,7 @@ struct hlsl_ir_constant *hlsl_new_uint_constant(struct hlsl_ctx *ctx, unsigned i

if (!(c = hlsl_alloc(ctx, sizeof(*c))))
         return NULL;
-    init_node(&c->node, HLSL_IR_CONSTANT, ctx->builtin_types.scalar[HLSL_TYPE_UINT], loc);
+    init_node(&c->node, HLSL_IR_CONSTANT, get_scalar_type(ctx, HLSL_TYPE_UINT), loc);
     c->value[0].u = n;
     return c;
 }
@@ -650,7 +650,7 @@ struct hlsl_ir_swizzle *hlsl_new_swizzle(struct hlsl_ctx *ctx, DWORD s, unsigned
     if (!(swizzle = hlsl_alloc(ctx, sizeof(*swizzle))))
         return NULL;
     init_node(&swizzle->node, HLSL_IR_SWIZZLE,
-            ctx->builtin_types.vector[val->data_type->base_type][components - 1], *loc);
+            get_vector_type(ctx, val->data_type->base_type, components), *loc);
     hlsl_src_from_node(&swizzle->val, val);
     swizzle->swizzle = s;
     return swizzle;
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index e401971e..2e6b1bf7 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -629,6 +629,34 @@ static inline void hlsl_release_string_buffer(struct hlsl_ctx *ctx, struct vkd3d
     vkd3d_string_buffer_release(&ctx->string_buffers, buffer);
 }

+static inline struct hlsl_type *get_scalar_type(const struct hlsl_ctx *ctx, enum hlsl_base_type base_type)
+{
+    return ctx->builtin_types.scalar[base_type];
+}
+
+static inline struct hlsl_type *get_vector_type(const struct hlsl_ctx *ctx, enum hlsl_base_type base_type,
+        unsigned int dimx)
+{
+    return ctx->builtin_types.vector[base_type][dimx - 1];
+}
+
+static inline struct hlsl_type *get_matrix_type(const struct hlsl_ctx *ctx, enum hlsl_base_type base_type,
+        unsigned int dimx, unsigned int dimy)
+{
+    return ctx->builtin_types.matrix[base_type][dimx - 1][dimy - 1];
+}
+
+static inline struct hlsl_type *get_numeric_type(const struct hlsl_ctx *ctx, enum hlsl_type_class type,
+        enum hlsl_base_type base_type, unsigned int dimx, unsigned int dimy)
+{
+    if (type == HLSL_CLASS_SCALAR)
+        return get_scalar_type(ctx, base_type);
+    else if (type == HLSL_CLASS_VECTOR)
+        return get_vector_type(ctx, base_type, dimx);
+    else
+        return get_matrix_type(ctx, base_type, dimx, dimy);
+}
+
 const char *debug_hlsl_expr_op(enum hlsl_ir_expr_op op);
 const char *debug_hlsl_type(struct hlsl_ctx *ctx, const struct hlsl_type *type);
 const char *debug_hlsl_writemask(unsigned int writemask);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index ae19cd1a..addb9c59 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1021,11 +1021,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type
         }
     }

-    if (type == HLSL_CLASS_SCALAR)
-        return ctx->builtin_types.scalar[base];
-    if (type == HLSL_CLASS_VECTOR)
-        return ctx->builtin_types.vector[base][dimx - 1];
-    return ctx->builtin_types.matrix[base][dimx - 1][dimy - 1];
+    return get_numeric_type(ctx, type, base, dimx, dimy);
 }

static struct hlsl_ir_expr *add_expr(struct hlsl_ctx *ctx, struct list *instrs, enum hlsl_ir_expr_op op,
@@ -1761,7 +1757,7 @@ static struct list *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type *type
         }

if (!(arg = add_implicit_conversion(ctx, params->instrs, arg,
-                ctx->builtin_types.vector[type->base_type][width - 1], &arg->loc)))
+                get_vector_type(ctx, type->base_type, width), &arg->loc)))
             continue;

if (!(store = hlsl_new_store(ctx, var, NULL, arg,
@@ -1816,9 +1812,9 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
         if (params->args_count >= 2)
             FIXME("Ignoring index and/or offset parameter(s).\n");

-        /* -1 for zero-indexing; +1 for the mipmap level */
+        /* +1 for the mipmap level */
         if (!(coords = add_implicit_conversion(ctx, instrs, params->args[0],
-                ctx->builtin_types.vector[HLSL_TYPE_INT][sampler_dim - 1 + 1], loc)))
+                get_vector_type(ctx, HLSL_TYPE_INT, sampler_dim + 1), loc)))
             return false;

if (!(load = hlsl_new_resource_load(ctx, object_type->e.resource_format, HLSL_RESOURCE_LOAD,
@@ -2500,7 +2496,7 @@ type:
                 YYABORT;
             }

-            $$ = ctx->builtin_types.vector[$3->base_type][$5 - 1];
+            $$ = get_vector_type(ctx, $3->base_type, $5);
         }
     | KW_MATRIX '<' type ',' C_INTEGER ',' C_INTEGER '>'
         {
@@ -2528,7 +2524,7 @@ type:
                 YYABORT;
             }

-            $$ = ctx->builtin_types.matrix[$3->base_type][$7 - 1][$5 - 1];
+            $$ = get_matrix_type(ctx, $3->base_type, $7, $5);
         }
     | KW_VOID
         {
@@ -2560,7 +2556,7 @@ type:
         }
     | texture_type
         {
-            $$ = hlsl_new_texture_type(ctx, $1, ctx->builtin_types.vector[HLSL_TYPE_FLOAT][4 - 1]);
+            $$ = hlsl_new_texture_type(ctx, $1, get_vector_type(ctx, HLSL_TYPE_FLOAT, 4));
         }
     | texture_type '<' type '>'
         {
@@ -2971,7 +2967,7 @@ primary_expr:

if (!(c = hlsl_alloc(ctx, sizeof(*c))))
                 YYABORT;
-            init_node(&c->node, HLSL_IR_CONSTANT, ctx->builtin_types.scalar[HLSL_TYPE_FLOAT], @1);
+            init_node(&c->node, HLSL_IR_CONSTANT, get_scalar_type(ctx, HLSL_TYPE_FLOAT), @1);
             c->value[0].f = $1;
             if (!($$ = make_list(ctx, &c->node)))
                 YYABORT;
@@ -2982,7 +2978,7 @@ primary_expr:

if (!(c = hlsl_alloc(ctx, sizeof(*c))))
                 YYABORT;
-            init_node(&c->node, HLSL_IR_CONSTANT, ctx->builtin_types.scalar[HLSL_TYPE_INT], @1);
+            init_node(&c->node, HLSL_IR_CONSTANT, get_scalar_type(ctx, HLSL_TYPE_INT), @1);
             c->value[0].i = $1;
             if (!($$ = make_list(ctx, &c->node)))
                 YYABORT;
@@ -2993,7 +2989,7 @@ primary_expr:

if (!(c = hlsl_alloc(ctx, sizeof(*c))))
                 YYABORT;
-            init_node(&c->node, HLSL_IR_CONSTANT, ctx->builtin_types.scalar[HLSL_TYPE_BOOL], @1);
+            init_node(&c->node, HLSL_IR_CONSTANT, get_scalar_type(ctx, HLSL_TYPE_BOOL), @1);
             c->value[0].b = $1;
             if (!($$ = make_list(ctx, &c->node)))
                 YYABORT;
@@ -3030,7 +3026,7 @@ primary_expr:
                 struct hlsl_ir_var *var;

if (!(var = hlsl_new_synthetic_var(ctx, "<state-block-expr>",
-                        ctx->builtin_types.scalar[HLSL_TYPE_INT], @1)))
+                        get_scalar_type(ctx, HLSL_TYPE_INT), @1)))
                     YYABORT;
                 if (!(load = hlsl_new_var_load(ctx, var, @1)))
                     YYABORT;
@@ -3116,7 +3112,7 @@ postfix_expr:
                 YYABORT;
             }

-            if (!(cast = hlsl_new_cast(ctx, index, ctx->builtin_types.scalar[HLSL_TYPE_UINT], &index->loc)))
+            if (!(cast = hlsl_new_cast(ctx, index, get_scalar_type(ctx, HLSL_TYPE_UINT), &index->loc)))
             {
                 destroy_instr_list($1);
                 YYABORT;

[PATCH vkd3d 2/6] vkd3d-shader/hlsl: Move the common shape computation to expr_common_shape.

[Giovanni Mascellani]

Function expr_common_shape can be used for boolean operators,
for which a common shape must be determined even if the base type
of the result is always bool.

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.y | 82 +++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index addb9c59..05573739 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -912,13 +912,9 @@ static enum hlsl_base_type expr_common_base_type(enum hlsl_base_type t1, enum hl
     return HLSL_TYPE_INT;
 }

-static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
-        struct vkd3d_shader_location *loc)
+static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
+        struct vkd3d_shader_location *loc, enum hlsl_type_class *type, unsigned int *dimx, unsigned int *dimy)
 {
-    enum hlsl_type_class type;
-    enum hlsl_base_type base;
-    unsigned int dimx, dimy;
-
     if (t1->type > HLSL_CLASS_LAST_NUMERIC)
     {
         struct vkd3d_string_buffer *string;
@@ -927,7 +923,7 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type
             hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
                     "Expression of type "%s" cannot be used in a numeric expression.", string->buffer);
         hlsl_release_string_buffer(ctx, string);
-        return NULL;
+        return false;
     }

if (t2->type > HLSL_CLASS_LAST_NUMERIC)
@@ -938,12 +934,9 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type
             hlsl_error(ctx, *loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
                     "Expression of type "%s" cannot be used in a numeric expression.", string->buffer);
         hlsl_release_string_buffer(ctx, string);
-        return NULL;
+        return false;
     }

-    if (hlsl_types_are_equal(t1, t2))
-        return t1;
-
     if (!expr_compatible_data_types(t1, t2))
     {
         struct vkd3d_string_buffer *t1_string = hlsl_type_to_string(ctx, t1);
@@ -955,28 +948,26 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type
                     t1_string->buffer, t2_string->buffer);
         hlsl_release_string_buffer(ctx, t1_string);
         hlsl_release_string_buffer(ctx, t2_string);
-        return NULL;
+        return false;
     }

-    base = expr_common_base_type(t1->base_type, t2->base_type);
-
     if (t1->dimx == 1 && t1->dimy == 1)
     {
-        type = t2->type;
-        dimx = t2->dimx;
-        dimy = t2->dimy;
+        *type = t2->type;
+        *dimx = t2->dimx;
+        *dimy = t2->dimy;
     }
     else if (t2->dimx == 1 && t2->dimy == 1)
     {
-        type = t1->type;
-        dimx = t1->dimx;
-        dimy = t1->dimy;
+        *type = t1->type;
+        *dimx = t1->dimx;
+        *dimy = t1->dimy;
     }
     else if (t1->type == HLSL_CLASS_MATRIX && t2->type == HLSL_CLASS_MATRIX)
     {
-        type = HLSL_CLASS_MATRIX;
-        dimx = min(t1->dimx, t2->dimx);
-        dimy = min(t1->dimy, t2->dimy);
+        *type = HLSL_CLASS_MATRIX;
+        *dimx = min(t1->dimx, t2->dimx);
+        *dimy = min(t1->dimy, t2->dimy);
     }
     else
     {
@@ -987,40 +978,55 @@ static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type
         max_dim_2 = max(t2->dimx, t2->dimy);
         if (t1->dimx * t1->dimy == t2->dimx * t2->dimy)
         {
-            type = HLSL_CLASS_VECTOR;
-            dimx = max(t1->dimx, t2->dimx);
-            dimy = 1;
+            *type = HLSL_CLASS_VECTOR;
+            *dimx = max(t1->dimx, t2->dimx);
+            *dimy = 1;
         }
         else if (max_dim_1 <= max_dim_2)
         {
-            type = t1->type;
-            if (type == HLSL_CLASS_VECTOR)
+            *type = t1->type;
+            if (*type == HLSL_CLASS_VECTOR)
             {
-                dimx = max_dim_1;
-                dimy = 1;
+                *dimx = max_dim_1;
+                *dimy = 1;
             }
             else
             {
-                dimx = t1->dimx;
-                dimy = t1->dimy;
+                *dimx = t1->dimx;
+                *dimy = t1->dimy;
             }
         }
         else
         {
-            type = t2->type;
-            if (type == HLSL_CLASS_VECTOR)
+            *type = t2->type;
+            if (*type == HLSL_CLASS_VECTOR)
             {
-                dimx = max_dim_2;
-                dimy = 1;
+                *dimx = max_dim_2;
+                *dimy = 1;
             }
             else
             {
-                dimx = t2->dimx;
-                dimy = t2->dimy;
+                *dimx = t2->dimx;
+                *dimy = t2->dimy;
             }
         }
     }

+    return true;
+}
+
+static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
+        struct vkd3d_shader_location *loc)
+{
+    enum hlsl_type_class type;
+    enum hlsl_base_type base;
+    unsigned int dimx, dimy;
+
+    if (!expr_common_shape(ctx, t1, t2, loc, &type, &dimx, &dimy))
+        return NULL;
+
+    base = expr_common_base_type(t1->base_type, t2->base_type);
+
     return get_numeric_type(ctx, type, base, dimx, dimy);
 }

[PATCH vkd3d 2/6] vkd3d-shader/hlsl: Move the common shape computation to expr_common_shape.

[Zebediah Figura]

On 10/14/21 8:37 AM, Giovanni Mascellani wrote:
> Function expr_common_shape can be used for boolean operators,
> for which a common shape must be determined even if the base type
> of the result is always bool.
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl.y | 82 +++++++++++++++++++++-------------------
>   1 file changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index addb9c59..05573739 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -912,13 +912,9 @@ static enum hlsl_base_type expr_common_base_type(enum hlsl_base_type t1, enum hl
>       return HLSL_TYPE_INT;
>   }
>
> -static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
> -
>    struct vkd3d_shader_location *loc)
>
> +static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
> -
>    struct vkd3d_shader_location *loc, enum hlsl_type_class *type, unsigned int *dimx, unsigned int *dimy)

Would it make more sense to instead pass the result of 
expr_common_base_type() to expr_common_shape()? I.e. something like

struct hlsl_type *expr_common_shape(ctx, t1, t2, 
expr_common_base_type(t1, t2));

or

struct hlsl_type *expr_common_shape(ctx, t1, t2, HLSL_TYPE_BOOL);

One of those things I think of when I see a function with a lot of 
output parameters :-)

[PATCH vkd3d 2/6] vkd3d-shader/hlsl: Move the common shape computation to expr_common_shape.

[Giovanni Mascellani]

Hi,

Il 14/10/21 18:09, Zebediah Figura ha scritto:
> Would it make more sense to instead pass the result of 
> expr_common_base_type() to expr_common_shape()? I.e. something like
>
> struct hlsl_type *expr_common_shape(ctx, t1, t2, 
> expr_common_base_type(t1, t2));
>
> or
>
> struct hlsl_type *expr_common_shape(ctx, t1, t2, HLSL_TYPE_BOOL);
>
> One of those things I think of when I see a function with a lot of 
> output parameters :-)

I can do that, if you really want, but I like it less. First because 
IMHO it uselessly subverts the dependencies between different pieces of 
data: the process of computing the result type consists of two parts, 
determining the base type and determining the shape. In principle they 
are nicely independent (well, I think HLSL has some very awful corner 
cases in which this does not happen, but for the moment we're pretending 
only the nice part exists), while reading the code you're proposing I 
would be led to think that determining the shape depends on which base 
type you worked out. I find this confusing.

Also, this means that, for example in the case of the comparison 
operations of patch 6/6, you have to call expr_common_shape twice, with 
two different base types. Ok, not a big deal, but why make the code more 
confusing for computing twice the same thing?

I don't like either passing three pointers to have three outputs from a 
function, but that's what you get when your language doesn't have decent 
support for anonymous product types (tuples). In Python the same thing 
would have been very straightforward and idiomatic. And defining a 
struct just to pass three values seems a bit excessive.

Does this make sense to you?

Thanks for the review, Giovanni.

[PATCH vkd3d 2/6] vkd3d-shader/hlsl: Move the common shape computation to expr_common_shape.

[Zebediah Figura]

On 10/14/21 12:37 PM, Giovanni Mascellani wrote:
> Hi,
>
> Il 14/10/21 18:09, Zebediah Figura ha scritto:
> > Would it make more sense to instead pass the result of
> > expr_common_base_type() to expr_common_shape()? I.e. something like
> >
> > struct hlsl_type *expr_common_shape(ctx, t1, t2,
> > expr_common_base_type(t1, t2));
> >
> > or
> >
> > struct hlsl_type *expr_common_shape(ctx, t1, t2, HLSL_TYPE_BOOL);
> >
> > One of those things I think of when I see a function with a lot of
> > output parameters :-)
>
> I can do that, if you really want, but I like it less. First because
> IMHO it uselessly subverts the dependencies between different pieces of
> data: the process of computing the result type consists of two parts,
> determining the base type and determining the shape. In principle they
> are nicely independent (well, I think HLSL has some very awful corner
> cases in which this does not happen, but for the moment we're pretending
> only the nice part exists), while reading the code you're proposing I
> would be led to think that determining the shape depends on which base
> type you worked out. I find this confusing.

In a sense, yeah, although I probably wouldn't call it 
expr_common_shape() either, which is probably part of the confusion.
> Also, this means that, for example in the case of the comparison
> operations of patch 6/6, you have to call expr_common_shape twice, with
> two different base types. Ok, not a big deal, but why make the code more
> confusing for computing twice the same thing?

Hmm, I suppose so...

Granted, you don't *have* to call expr_common_shape() twice; you can 
take the prior type and change it to BOOL.
> I don't like either passing three pointers to have three outputs from a
> function, but that's what you get when your language doesn't have decent
> support for anonymous product types (tuples). In Python the same thing
> would have been very straightforward and idiomatic. And defining a
> struct just to pass three values seems a bit excessive.

Well, that or proper out parameters, I guess, like HLSL ;-)

Not that HLSL is a pinnacle of good language design, but I'm sure if 
this weren't C I'd find it more idiomatic.

As it is I still have a mild preference for avoiding the output 
parameters, just for the sake of what I see as idiomatic code, but I 
won't object if you keep it the way it is.

[PATCH vkd3d 2/6] vkd3d-shader/hlsl: Move the common shape computation to expr_common_shape.

[Giovanni Mascellani]

Hi,

Il 15/10/21 04:39, Zebediah Figura ha scritto:
> Not that HLSL is a pinnacle of good language design, but I'm sure if 
> this weren't C I'd find it more idiomatic.

That's funny: passing pointer is precisely what I would consider the 
idiomatic C way to return more than one value. Even for HLSL, out 
parameter essentially have the semantics of passing by reference instead 
of by value, AFAIK.

(we still have the Microsoft way, though: return type << 16 | dimx << 8 
| dimy, would that be better? :-P)
> As it is I still have a mild preference for avoiding the output 
> parameters, just for the sake of what I see as idiomatic code, but I 
> won't object if you keep it the way it is.

I think in this case I'd prefer to keep the way it is, but I'll concede 
for 5/6, which hopefully can be a good compromise for you.

Giovanni.

[PATCH vkd3d 3/6] vkd3d-shader/hlsl: Simplify common shape computation.

[Giovanni Mascellani]

When t1 is a vector type, it's already supposed to have dimx == max_dim_1
and dimy == 1, and the same for t2.

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.y | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 05573739..3119380c 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -985,30 +985,14 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
         else if (max_dim_1 <= max_dim_2)
         {
             *type = t1->type;
-            if (*type == HLSL_CLASS_VECTOR)
-            {
-                *dimx = max_dim_1;
-                *dimy = 1;
-            }
-            else
-            {
-                *dimx = t1->dimx;
-                *dimy = t1->dimy;
-            }
+            *dimx = t1->dimx;
+            *dimy = t1->dimy;
         }
         else
         {
             *type = t2->type;
-            if (*type == HLSL_CLASS_VECTOR)
-            {
-                *dimx = max_dim_2;
-                *dimy = 1;
-            }
-            else
-            {
-                *dimx = t2->dimx;
-                *dimy = t2->dimy;
-            }
+            *dimx = t2->dimx;
+            *dimy = t2->dimy;
         }
     }

[PATCH vkd3d 3/6] vkd3d-shader/hlsl: Simplify common shape computation.

[Zebediah Figura]

Looks correct to me. The amount of thinking I had to do to make sure of 
that is a good sign that this cleanup really is needed :-/

[PATCH vkd3d 4/6] vkd3d-shader/hlsl: Fix common shape computation.

[Giovanni Mascellani]

Matrices of shape 1xN always decay to vectors of length N.

Moreover, the assumption about the size of matrices is not correct:
it is legitimate to compose a matrix 2x2 with a vector of length 4,
in which case it appears that the result has the shape of the first
(leftmost) operand. Even for matrices 1xN or Nx1, the result is not
always a vector: in general it has the shape of the first operand
again, which gets possibly decayed to a vector by the rule above.

This algorithm is unfortunately not complete yet: sometimes, but not
always, vectors of length 1 are decayed to scalars.

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.y | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 3119380c..111db8cc 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -971,18 +971,7 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
     }
     else
     {
-        /* Two vectors or a vector and a matrix (matrix must be 1xn or nx1) */
-        unsigned int max_dim_1, max_dim_2;
-
-        max_dim_1 = max(t1->dimx, t1->dimy);
-        max_dim_2 = max(t2->dimx, t2->dimy);
-        if (t1->dimx * t1->dimy == t2->dimx * t2->dimy)
-        {
-            *type = HLSL_CLASS_VECTOR;
-            *dimx = max(t1->dimx, t2->dimx);
-            *dimy = 1;
-        }
-        else if (max_dim_1 <= max_dim_2)
+        if (t1->dimx * t1->dimy <= t2->dimx * t2->dimy)
         {
             *type = t1->type;
             *dimx = t1->dimx;
@@ -996,6 +985,9 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
         }
     }

+    if (*type == HLSL_CLASS_MATRIX && *dimy == 1)
+        *type = HLSL_CLASS_VECTOR;
+
     return true;
 }

[PATCH vkd3d 4/6] vkd3d-shader/hlsl: Fix common shape computation.

[Zebediah Figura]

On 10/14/21 8:37 AM, Giovanni Mascellani wrote:
> Matrices of shape 1xN always decay to vectors of length N.
>
> Moreover, the assumption about the size of matrices is not correct:
> it is legitimate to compose a matrix 2x2 with a vector of length 4,
> in which case it appears that the result has the shape of the first
> (leftmost) operand. Even for matrices 1xN or Nx1, the result is not
> always a vector: in general it has the shape of the first operand
> again, which gets possibly decayed to a vector by the rule above.

"Fix X" is not usually a very good commit message, and here as often it 
is a sign that multiple things are being done at once.

I.e. I think the "matrices decay to vectors" part makes sense as a 
separate commit (e.g. I guess float4x1 + float4x2 = float4 would be 
fixed by that case).
> This algorithm is unfortunately not complete yet: sometimes, but not
> always, vectors of length 1 are decayed to scalars.
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl.y | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 3119380c..111db8cc 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -971,18 +971,7 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
>       }
>       else
>       {
> -
>    /* Two vectors or a vector and a matrix (matrix must be 1xn or nx1) */
> -
>    unsigned int max_dim_1, max_dim_2;
> -
> -
>    max_dim_1 = max(t1->dimx, t1->dimy);
> -
>    max_dim_2 = max(t2->dimx, t2->dimy);
> -
>    if (t1->dimx * t1->dimy == t2->dimx * t2->dimy)
> -
>    {
> -
>        *type = HLSL_CLASS_VECTOR;
> -
>        *dimx = max(t1->dimx, t2->dimx);
> -
>        *dimy = 1;
> -
>    }
> -
>    else if (max_dim_1 <= max_dim_2)
> -
>    if (t1->dimx * t1->dimy <= t2->dimx * t2->dimy)
>     {
>         *type = t1->type;
>         *dimx = t1->dimx;
>
> @@ -996,6 +985,9 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
>           }
>       }
> - if (*type == HLSL_CLASS_MATRIX && *dimy == 1)
> -
>    *type = HLSL_CLASS_VECTOR;
> -
> return true;
> }

[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Giovanni Mascellani]

In HLSL the rules used to determine the type of a composite expression
change depending on the operator. This commit changes add_expr so that
it accepts both the types to which the operands have to be casted
and the type that the final expression must have (which is not
necessarily the cast type).

For example, when comparing a half with an int, both of them will
have to be casted to float, though the type of the expression as a
whole will be bool.

Similarly, it is not always the case that all operands have to be
casted to the same type, therefore the interface leaves room for
specifying a different cast type for each operand. For example,
the first operand of the ternary operator must be casted to bool,
while the other two must be casted to their common type, which is
then the output type of the whole expression.

The add_*_last helpers behave similarly as their counterpart without
"_last", except that the last node in each instruction list is used
as an argument.

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.y | 144 ++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 78 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 111db8cc..baff757e 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -991,52 +991,28 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct
     return true;
 }

-static struct hlsl_type *expr_common_type(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct hlsl_type *t2,
-        struct vkd3d_shader_location *loc)
-{
-    enum hlsl_type_class type;
-    enum hlsl_base_type base;
-    unsigned int dimx, dimy;
-
-    if (!expr_common_shape(ctx, t1, t2, loc, &type, &dimx, &dimy))
-        return NULL;
-
-    base = expr_common_base_type(t1->base_type, t2->base_type);
-
-    return get_numeric_type(ctx, type, base, dimx, dimy);
-}
-
 static struct hlsl_ir_expr *add_expr(struct hlsl_ctx *ctx, struct list *instrs, enum hlsl_ir_expr_op op,
-        struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS], struct vkd3d_shader_location *loc)
+        struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS], struct hlsl_type *cast_types[HLSL_MAX_OPERANDS],
+        struct hlsl_type *type, struct vkd3d_shader_location *loc)
 {
     struct hlsl_ir_expr *expr;
-    struct hlsl_type *type;
     unsigned int i;

-    type = operands[0]->data_type;
-    for (i = 1; i < HLSL_MAX_OPERANDS; ++i)
-    {
-        if (!operands[i])
-            break;
-        type = expr_common_type(ctx, type, operands[i]->data_type, loc);
-        if (!type)
-            return NULL;
-    }
     for (i = 0; i < HLSL_MAX_OPERANDS; ++i)
     {
         struct hlsl_ir_expr *cast;

if (!operands[i])
             break;
-        if (hlsl_types_are_equal(operands[i]->data_type, type))
+        if (hlsl_types_are_equal(operands[i]->data_type, cast_types[i]))
             continue;
         if (operands[i]->data_type->dimx * operands[i]->data_type->dimy != 1
-                && operands[i]->data_type->dimx * operands[i]->data_type->dimy != type->dimx * type->dimy)
+                && operands[i]->data_type->dimx * operands[i]->data_type->dimy != cast_types[i]->dimx * cast_types[i]->dimy)
             hlsl_warning(ctx, operands[i]->loc, VKD3D_SHADER_WARNING_HLSL_IMPLICIT_TRUNCATION,
                     "Implicit truncation of %s type.",
                     operands[i]->data_type->type == HLSL_CLASS_VECTOR ? "vector" : "matrix");

-        if (!(cast = hlsl_new_cast(ctx, operands[i], type, &operands[i]->loc)))
+        if (!(cast = hlsl_new_cast(ctx, operands[i], cast_types[i], &operands[i]->loc)))
             return NULL;
         list_add_after(&operands[i]->entry, &cast->node.entry);
         operands[i] = &cast->node;
@@ -1053,26 +1029,52 @@ static struct hlsl_ir_expr *add_expr(struct hlsl_ctx *ctx, struct list *instrs,
     return expr;
 }

-static struct list *add_unary_expr(struct hlsl_ctx *ctx, struct list *instrs,
-        enum hlsl_ir_expr_op op, struct vkd3d_shader_location loc)
+static struct hlsl_ir_expr *add_unary_arithmetic_expr(struct hlsl_ctx *ctx, struct list *instrs,
+        enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg, struct vkd3d_shader_location *loc)
 {
-    struct hlsl_ir_node *expr;
+    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {arg};
+    struct hlsl_type *cast_types[HLSL_MAX_OPERANDS] = {arg->data_type};
+
+    return add_expr(ctx, instrs, op, args, cast_types, arg->data_type, loc);
+}
+
+static struct list *add_unary_arithmetic_expr_last(struct hlsl_ctx *ctx, struct list *instrs,
+        enum hlsl_ir_expr_op op, struct vkd3d_shader_location *loc)
+{
+    add_unary_arithmetic_expr(ctx, instrs, op, node_from_list(instrs), loc);

-    if (!(expr = hlsl_new_unary_expr(ctx, op, node_from_list(instrs), loc)))
-        return NULL;
-    list_add_tail(instrs, &expr->entry);
     return instrs;
 }

-static struct list *add_binary_expr(struct hlsl_ctx *ctx, struct list *list1, struct list *list2,
-        enum hlsl_ir_expr_op op, struct vkd3d_shader_location loc)
+static struct hlsl_ir_expr *add_binary_arithmetic_expr(struct hlsl_ctx *ctx, struct list *instrs,
+        enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2,
+        struct vkd3d_shader_location *loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {node_from_list(list1), node_from_list(list2)};
+    struct hlsl_type *cast_types[HLSL_MAX_OPERANDS];
+    enum hlsl_base_type base = expr_common_base_type(arg1->data_type->base_type, arg2->data_type->base_type);
+    enum hlsl_type_class type;
+    unsigned int dimx, dimy;
+    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {arg1, arg2};
+
+    if (!expr_common_shape(ctx, arg1->data_type, arg2->data_type, loc, &type, &dimx, &dimy))
+        return NULL;
+
+    cast_types[0] = get_numeric_type(ctx, type, base, dimx, dimy);
+    cast_types[1] = cast_types[0];

-    list_move_tail(list1, list2);
-    vkd3d_free(list2);
-    add_expr(ctx, list1, op, args, &loc);
-    return list1;
+    return add_expr(ctx, instrs, op, args, cast_types, cast_types[0], loc);
+}
+
+static struct list *add_binary_arithmetic_expr_last(struct hlsl_ctx *ctx, struct list *instrs1, struct list *instrs2,
+        enum hlsl_ir_expr_op op, struct vkd3d_shader_location *loc)
+{
+    struct hlsl_ir_node *arg1 = node_from_list(instrs1), *arg2 = node_from_list(instrs2);
+
+    list_move_tail(instrs1, instrs2);
+    vkd3d_free(instrs2);
+    add_binary_arithmetic_expr(ctx, instrs1, op, arg1, arg2, loc);
+
+    return instrs1;
 }

static enum hlsl_ir_expr_op op_from_assignment(enum parse_assign_op op)
@@ -1141,22 +1143,20 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct list *in

if (assign_op == ASSIGN_OP_SUB)
     {
-        struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {rhs};
         struct hlsl_ir_expr *expr;

-        if (!(expr = add_expr(ctx, instrs, HLSL_OP1_NEG, args, &rhs->loc)))
+        if (!(expr = add_unary_arithmetic_expr(ctx, instrs, HLSL_OP1_NEG, rhs, &rhs->loc)))
             return NULL;
         rhs = &expr->node;
         assign_op = ASSIGN_OP_ADD;
     }
     if (assign_op != ASSIGN_OP_ASSIGN)
     {
-        struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {lhs, rhs};
         enum hlsl_ir_expr_op op = op_from_assignment(assign_op);
         struct hlsl_ir_expr *expr;

assert(op);
-        if (!(expr = add_expr(ctx, instrs, op, args, &rhs->loc)))
+        if (!(expr = add_binary_arithmetic_expr(ctx, instrs, op, lhs, rhs, &rhs->loc)))
             return NULL;
         rhs = &expr->node;
     }
@@ -1557,37 +1557,29 @@ static const struct hlsl_ir_function_decl *find_function_call(struct hlsl_ctx *c
 static bool intrinsic_abs(struct hlsl_ctx *ctx,
         const struct parse_initializer *params, struct vkd3d_shader_location loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {params->args[0]};
-
-    return !!add_expr(ctx, params->instrs, HLSL_OP1_ABS, args, &loc);
+    return !!add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_ABS, params->args[0], &loc);
 }

static bool intrinsic_clamp(struct hlsl_ctx *ctx,
         const struct parse_initializer *params, struct vkd3d_shader_location loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {params->args[0], params->args[1]};
     struct hlsl_ir_expr *max;

-    if (!(max = add_expr(ctx, params->instrs, HLSL_OP2_MAX, args, &loc)))
+    if (!(max = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MAX, params->args[0], params->args[1], &loc)))
         return false;

-    args[0] = &max->node;
-    args[1] = params->args[2];
-    return !!add_expr(ctx, params->instrs, HLSL_OP2_MIN, args, &loc);
+    return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MIN, &max->node, params->args[2], &loc);
 }

static bool intrinsic_max(struct hlsl_ctx *ctx,
         const struct parse_initializer *params, struct vkd3d_shader_location loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {params->args[0], params->args[1]};
-
-    return !!add_expr(ctx, params->instrs, HLSL_OP2_MAX, args, &loc);
+    return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MAX, params->args[0], params->args[1], &loc);
 }

static bool intrinsic_pow(struct hlsl_ctx *ctx,
         const struct parse_initializer *params, struct vkd3d_shader_location loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {0};
     struct hlsl_ir_node *log, *exp;
     struct hlsl_ir_expr *mul;

@@ -1595,9 +1587,7 @@ static bool intrinsic_pow(struct hlsl_ctx *ctx,
         return false;
     list_add_tail(params->instrs, &log->entry);

-    args[0] = params->args[1];
-    args[1] = log;
-    if (!(mul = add_expr(ctx, params->instrs, HLSL_OP2_MUL, args, &loc)))
+    if (!(mul = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, params->args[1], log, &loc)))
         return false;

if (!(exp = hlsl_new_unary_expr(ctx, HLSL_OP1_EXP2, &mul->node, loc)))
@@ -1609,9 +1599,7 @@ static bool intrinsic_pow(struct hlsl_ctx *ctx,
 static bool intrinsic_saturate(struct hlsl_ctx *ctx,
         const struct parse_initializer *params, struct vkd3d_shader_location loc)
 {
-    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {params->args[0]};
-
-    return !!add_expr(ctx, params->instrs, HLSL_OP1_SAT, args, &loc);
+    return !!add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_SAT, params->args[0], &loc);
 }

static const struct intrinsic_function
@@ -3188,15 +3176,15 @@ unary_expr:
         }
     | '-' unary_expr
         {
-            $$ = add_unary_expr(ctx, $2, HLSL_OP1_NEG, @1);
+            $$ = add_unary_arithmetic_expr_last(ctx, $2, HLSL_OP1_NEG, &@1);
         }
     | '~' unary_expr
         {
-            $$ = add_unary_expr(ctx, $2, HLSL_OP1_BIT_NOT, @1);
+            $$ = add_unary_arithmetic_expr_last(ctx, $2, HLSL_OP1_BIT_NOT, &@1);
         }
     | '!' unary_expr
         {
-            $$ = add_unary_expr(ctx, $2, HLSL_OP1_LOGIC_NOT, @1);
+            $$ = add_unary_arithmetic_expr_last(ctx, $2, HLSL_OP1_LOGIC_NOT, &@1);
         }
     /* var_modifiers is necessary to avoid shift/reduce conflicts. */
     | '(' var_modifiers type arrays ')' unary_expr
@@ -3244,22 +3232,22 @@ mul_expr:
       unary_expr
     | mul_expr '*' unary_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_MUL, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_MUL, &@2);
         }
     | mul_expr '/' unary_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_DIV, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_DIV, &@2);
         }
     | mul_expr '%' unary_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_MOD, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_MOD, &@2);
         }

add_expr:
       mul_expr
     | add_expr '+' mul_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_ADD, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_ADD, &@2);
         }
     | add_expr '-' mul_expr
         {
@@ -3268,7 +3256,7 @@ add_expr:
             if (!(neg = hlsl_new_unary_expr(ctx, HLSL_OP1_NEG, node_from_list($3), @2)))
                 YYABORT;
             list_add_tail($3, &neg->entry);
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_ADD, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_ADD, &@2);
         }

shift_expr:
@@ -3286,30 +3274,30 @@ relational_expr:
       shift_expr
     | relational_expr '<' shift_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_LESS, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_LESS, &@2);
         }
     | relational_expr '>' shift_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_GREATER, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_GREATER, &@2);
         }
     | relational_expr OP_LE shift_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_LEQUAL, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_LEQUAL, &@2);
         }
     | relational_expr OP_GE shift_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_GEQUAL, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_GEQUAL, &@2);
         }

equality_expr:
       relational_expr
     | equality_expr OP_EQ relational_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_EQUAL, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_EQUAL, &@2);
         }
     | equality_expr OP_NE relational_expr
         {
-            $$ = add_binary_expr(ctx, $1, $3, HLSL_OP2_NEQUAL, @2);
+            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_NEQUAL, &@2);
         }

bitand_expr:

[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Zebediah Figura]

On 10/14/21 8:37 AM, Giovanni Mascellani wrote:
> In HLSL the rules used to determine the type of a composite expression
> change depending on the operator. This commit changes add_expr so that
> it accepts both the types to which the operands have to be casted
> and the type that the final expression must have (which is not
> necessarily the cast type).
>
> For example, when comparing a half with an int, both of them will
> have to be casted to float, though the type of the expression as a
> whole will be bool.
>
> Similarly, it is not always the case that all operands have to be
> casted to the same type, therefore the interface leaves room for
> specifying a different cast type for each operand. For example,
> the first operand of the ternary operator must be casted to bool,
> while the other two must be casted to their common type, which is
> then the output type of the whole expression.

Yeah, but the ternary operator is a special case. I don't think it 
should result in an HLSL_IR_EXPR. Probably we should synthesize a 
temporary variable, store to it inside of a conditional, and load from it.

I'm still not convinced that casting in add_expr() is better than 
casting in the caller, either.
> The add_*_last helpers behave similarly as their counterpart without
> "_last", except that the last node in each instruction list is used
> as an argument.

Yeah, I still don't like this name. "last" makes me think that the 
instruction will be added to the end of a list, or something. "merge" or 
"combine" would be a lot better, but it's a bit awkward for the unary 
helpers. But then again maybe we don't need the unary helpers, and we 
can just as easily just spell out node_from_list() inline. There are 
only three such cases after all (and I guess they're not even all the 
same kind of operation).

Or "parse", I guess, to symbolize it's done as part of parsing, although 
that's not great either.
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl.y | 144 ++++++++++++++++++---------------------
>   1 file changed, 66 insertions(+), 78 deletions(-)

I think at least splitting the "cast_types" part out of this patch would 
make it easier to read.

[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Giovanni Mascellani]

Hi,

Il 14/10/21 18:32, Zebediah Figura ha scritto:
> Yeah, but the ternary operator is a special case. I don't think it 
> should result in an HLSL_IR_EXPR. Probably we should synthesize a 
> temporary variable, store to it inside of a conditional, and load from it.

Why? I don't know in SM1, but at least in SM4 we have a nice opcode 
(MOVC) that does just what the ternary operator needs to do, in a way 
that is probably more efficient (or at least not less efficient) than 
doing an actual branch. Also, I have a few other patches in my queue 
that use MOVC for other things (like implementing casts from bool), so 
I'd like to have it in the IR (not really related to this patch, though).
> I'm still not convinced that casting in add_expr() is better than 
> casting in the caller, either.

From my point of view it makes the code more declarative and therefore 
easier to read. However apparently we have different tastes, and I don't 
have a too strong opinion.
> > The add_*_last helpers behave similarly as their counterpart without
> > "_last", except that the last node in each instruction list is used
> > as an argument.
>
> Yeah, I still don't like this name. "last" makes me think that the 
> instruction will be added to the end of a list, or something. "merge" or 
> "combine" would be a lot better, but it's a bit awkward for the unary 
> helpers. But then again maybe we don't need the unary helpers, and we 
> can just as easily just spell out node_from_list() inline. There are 
> only three such cases after all (and I guess they're not even all the 
> same kind of operation).
>
> Or "parse", I guess, to symbolize it's done as part of parsing, although 
> that's not great either.

I don't like it either, but neither I like better one of your 
alternatives, nor I could find anything better. Among your suggestions I 
would pick "parse", but if you eventually settle for something else just 
tell me.
> I think at least splitting the "cast_types" part out of this patch would 
> make it easier to read.

Ok, will do.

Thanks again, Giovanni.

[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Zebediah Figura]

On 10/14/21 12:52 PM, Giovanni Mascellani wrote:
> Hi,
>
> Il 14/10/21 18:32, Zebediah Figura ha scritto:
> > Yeah, but the ternary operator is a special case. I don't think it
> > should result in an HLSL_IR_EXPR. Probably we should synthesize a
> > temporary variable, store to it inside of a conditional, and load from it.
>
> Why? I don't know in SM1, but at least in SM4 we have a nice opcode
> (MOVC) that does just what the ternary operator needs to do, in a way
> that is probably more efficient (or at least not less efficient) than
> doing an actual branch. Also, I have a few other patches in my queue
> that use MOVC for other things (like implementing casts from bool), so
> I'd like to have it in the IR (not really related to this patch, though).

Well, the usual answer, which is that keeping the scope of the IR low 
makes it easier to work with.

Maybe there's an argument for having a ternary/movc in the IR, but even 
if there is, I'm not sure we really want to make add_expr() more complex 
just for that. Are there any other expressions that (might) take 
operands of non-uniform types?
> > I'm still not convinced that casting in add_expr() is better than
> > casting in the caller, either.
>
> From my point of view it makes the code more declarative and therefore
> easier to read. However apparently we have different tastes, and I don't
> have a too strong opinion.

I guess it's not clear to me that it's actually making the caller's job 
easier, and in lieu of that I'm inclined to prefer modularity. Not to 
get too philosophical, but while I'm a fan of declarativity, I tend to 
shy away from giving a function too many parameters if I can avoid it.
> > > The add_*_last helpers behave similarly as their counterpart without
> > > "_last", except that the last node in each instruction list is used
> > > as an argument.
> >
> > Yeah, I still don't like this name. "last" makes me think that the
> > instruction will be added to the end of a list, or something. "merge" or
> > "combine" would be a lot better, but it's a bit awkward for the unary
> > helpers. But then again maybe we don't need the unary helpers, and we
> > can just as easily just spell out node_from_list() inline. There are
> > only three such cases after all (and I guess they're not even all the
> > same kind of operation).
> >
> > Or "parse", I guess, to symbolize it's done as part of parsing, although
> > that's not great either.
>
> I don't like it either, but neither I like better one of your
> alternatives, nor I could find anything better. Among your suggestions I
> would pick "parse", but if you eventually settle for something else just
> tell me.

I would prefer "merge" if we can find a way to avoid needing it for 
unary helpers. Does that latter part seem too unpalatable?
> > I think at least splitting the "cast_types" part out of this patch would
> > make it easier to read.
>
> Ok, will do.
>
> Thanks again, Giovanni.

[PATCH vkd3d 5/6] vkd3d-shader/hlsl: Introduce specialized helpers for arithmetic operations.

[Matteo Bruni]

On Fri, Oct 15, 2021 at 4:54 AM Zebediah Figura  wrote:
> On 10/14/21 12:52 PM, Giovanni Mascellani wrote:
> > Hi,
> >
> > Il 14/10/21 18:32, Zebediah Figura ha scritto:
> > > Yeah, but the ternary operator is a special case. I don't think it
> > > should result in an HLSL_IR_EXPR. Probably we should synthesize a
> > > temporary variable, store to it inside of a conditional, and load from it.
> >
> > Why? I don't know in SM1, but at least in SM4 we have a nice opcode
> > (MOVC) that does just what the ternary operator needs to do, in a way
> > that is probably more efficient (or at least not less efficient) than
> > doing an actual branch. Also, I have a few other patches in my queue
> > that use MOVC for other things (like implementing casts from bool), so
> > I'd like to have it in the IR (not really related to this patch, though).
>
> Well, the usual answer, which is that keeping the scope of the IR low
> makes it easier to work with.
>
> Maybe there's an argument for having a ternary/movc in the IR, but even
> if there is, I'm not sure we really want to make add_expr() more complex
> just for that. Are there any other expressions that (might) take
> operands of non-uniform types?

I agree with all of this, but let me go one step further. In general
the (high-level) IR should express the program using the most basic
elements, then the compiler will figure out how to best map the
program to the output bytecode. For the ternary operator, ideally we
generate an IF in the compiler frontend (e.g. while parsing), then
during instruction selection we notice that we can use MOVC to
implement that particular IF so we do that. Or we lower suitable IFs
to MOVC in a separate pass if it becomes too complicated (but that
would make the most sense as a lower level-IR transformation).

That said, we can certainly compromise and take a more practical route
where it makes sense. The ternary operator doesn't feel like one such
case though, mainly because there is a good chance it would complicate
the rest of the compiler. For example, if some uses of the ternary
operator can't be mapped to MOVC, we potentially need to handle the
ternary op expression in all the passes that care about branches. Or
lower them to regular branches anyway.

I guess this might seem at odds with e.g. my previous suggestion to
lower matrix multiplication directly to a bunch of DP4s or MADs, but
those are pretty regular operations by comparison.

[PATCH vkd3d 6/6] vkd3d-shader/hlsl: Properly compute types of comparison expressions.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani 
---
 libs/vkd3d-shader/hlsl.y | 44 ++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index baff757e..100aa96f 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1077,6 +1077,38 @@ static struct list *add_binary_arithmetic_expr_last(struct hlsl_ctx *ctx, struct
     return instrs1;
 }

+static struct hlsl_ir_expr *add_binary_comparison_expr(struct hlsl_ctx *ctx, struct list *instrs,
+        enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2,
+        struct vkd3d_shader_location *loc)
+{
+    struct hlsl_type *cast_types[HLSL_MAX_OPERANDS], *ret_type;
+    enum hlsl_base_type base = expr_common_base_type(arg1->data_type->base_type, arg2->data_type->base_type);
+    enum hlsl_type_class type;
+    unsigned int dimx, dimy;
+    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {arg1, arg2};
+
+    if (!expr_common_shape(ctx, arg1->data_type, arg2->data_type, loc, &type, &dimx, &dimy))
+        return NULL;
+
+    cast_types[0] = get_numeric_type(ctx, type, base, dimx, dimy);
+    cast_types[1] = cast_types[0];
+    ret_type = get_numeric_type(ctx, type, HLSL_TYPE_BOOL, dimx, dimy);
+
+    return add_expr(ctx, instrs, op, args, cast_types, ret_type, loc);
+}
+
+static struct list *add_binary_comparison_expr_last(struct hlsl_ctx *ctx, struct list *instrs1, struct list *instrs2,
+        enum hlsl_ir_expr_op op, struct vkd3d_shader_location *loc)
+{
+    struct hlsl_ir_node *arg1 = node_from_list(instrs1), *arg2 = node_from_list(instrs2);
+
+    list_move_tail(instrs1, instrs2);
+    vkd3d_free(instrs2);
+    add_binary_comparison_expr(ctx, instrs1, op, arg1, arg2, loc);
+
+    return instrs1;
+}
+
 static enum hlsl_ir_expr_op op_from_assignment(enum parse_assign_op op)
 {
     static const enum hlsl_ir_expr_op ops[] =
@@ -3274,30 +3306,30 @@ relational_expr:
       shift_expr
     | relational_expr '<' shift_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_LESS, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_LESS, &@2);
         }
     | relational_expr '>' shift_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_GREATER, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_GREATER, &@2);
         }
     | relational_expr OP_LE shift_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_LEQUAL, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_LEQUAL, &@2);
         }
     | relational_expr OP_GE shift_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_GEQUAL, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_GEQUAL, &@2);
         }

equality_expr:
       relational_expr
     | equality_expr OP_EQ relational_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_EQUAL, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_EQUAL, &@2);
         }
     | equality_expr OP_NE relational_expr
         {
-            $$ = add_binary_arithmetic_expr_last(ctx, $1, $3, HLSL_OP2_NEQUAL, &@2);
+            $$ = add_binary_comparison_expr_last(ctx, $1, $3, HLSL_OP2_NEQUAL, &@2);
         }

bitand_expr:

[Zebediah Figura]

Can you please make sure that all of the HLSL helpers exported from a 
file begin with hlsl_?