[PATCH vkd3d v2] vkd3d-shader/hlsl: Return the shape of the left operand when appropriate.

[Giovanni Mascellani]

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.

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

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index bfe2bbb9..c42e372e 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;

[Giovanni Mascellani]

This supersedes 217302. Also, 217464, from the same patch set, is revoked.

Thanks, Giovanni.

Il 19/10/21 09:32, Giovanni Mascellani ha scritto:
> 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.
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl.y | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index bfe2bbb9..c42e372e 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;

[Zebediah Figura]

Signed-off-by: Zebediah Figura

[Matteo Bruni]

Signed-off-by: Matteo Bruni 
---
A detail that's not obvious from the patch is that
expr_compatible_data_types() already expects (and accepts) the case
where a vector and a matrix with the same component count are used
together, so this in practice fixes expr_common_shape() to conform to
that.

[Giovanni Mascellani]

Hi,

Il 21/10/21 16:17, Matteo Bruni ha scritto:
> A detail that's not obvious from the patch is that
> expr_compatible_data_types() already expects (and accepts) the case
> where a vector and a matrix with the same component count are used
> together, so this in practice fixes expr_common_shape() to conform to
> that.

Just FTR, in general I like to keep as close as possible the point where 
a certain precondition is checked and the point where the same condition 
is used, as I find that code is more readable that way. It could be 
argued that the mismatch between what expr_compatible_data_types() 
checks and the assumption in expr_common_shape() might have been due to 
the fact that there was too much between where the condition was checked 
and where it was used.

At some point I had a patch to fix expr_common_shape() in order to 
conform to this principle, but it was broken for other reasons, and 
eventually it bitrotted and I dropped it. But if you think that change 
is valuable, I can rewrite it.

Giovanni.

[Matteo Bruni]

On Thu, Oct 21, 2021 at 4:47 PM Giovanni Mascellani
 wrote:
> Hi,
>
> Il 21/10/21 16:17, Matteo Bruni ha scritto:
> > A detail that's not obvious from the patch is that
> > expr_compatible_data_types() already expects (and accepts) the case
> > where a vector and a matrix with the same component count are used
> > together, so this in practice fixes expr_common_shape() to conform to
> > that.
>
> Just FTR, in general I like to keep as close as possible the point where
> a certain precondition is checked and the point where the same condition
> is used, as I find that code is more readable that way. It could be
> argued that the mismatch between what expr_compatible_data_types()
> checks and the assumption in expr_common_shape() might have been due to
> the fact that there was too much between where the condition was checked
> and where it was used.
>
> At some point I had a patch to fix expr_common_shape() in order to
> conform to this principle, but it was broken for other reasons, and
> eventually it bitrotted and I dropped it. But if you think that change
> is valuable, I can rewrite it.

Yeah, in general I agree with the principle. WRT this specific case,
it's probably not a big deal. You're going to touch related code, if
the fixup patch comes out naturally I'm happy to review it. Otherwise,
probably it's not worth the effort to go out of your way to write it.