[PATCH vkd3d v2] vkd3d-shader/hlsl: Return the shape of the left operand when appropriate.
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 <gmascellani(a)codeweavers.com> --- 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; -- 2.33.0
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 <gmascellani(a)codeweavers.com> --- 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;
Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> --- 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.
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.
On Thu, Oct 21, 2021 at 4:47 PM Giovanni Mascellani <gmascellani(a)codeweavers.com> 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.
participants (4)
-
Giovanni Mascellani -
Matteo Bruni -
Matteo Bruni -
Zebediah Figura