[PATCH v3 0/1] MR336: vkd3d-shader/hlsl: Fix float modulus for vector arguments.

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov

--
  v3: vkd3d-shader/hlsl: Fix float modulus for vector arguments.

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336

[PATCH v3 1/1] vkd3d-shader/hlsl: Fix float modulus for vector arguments.

[Nikolay Sivov]

From: Nikolay Sivov

Signed-off-by: Nikolay Sivov 
---
 libs/vkd3d-shader/hlsl_codegen.c                |  7 ++++++-
 tests/hlsl/arithmetic-float-uniform.shader_test | 14 ++++++++++++++
 tests/hlsl/arithmetic-float.shader_test         | 13 +++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 132f44a2a..65c68d460 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -2719,6 +2719,7 @@ static bool lower_float_modulus(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr
 {
     struct hlsl_ir_node *arg1, *arg2, *mul1, *neg1, *ge, *neg2, *div, *mul2, *frc, *cond, *one, *mul3;
     struct hlsl_type *type = instr->data_type, *btype;
+    struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS];
     struct hlsl_constant_value one_value;
     struct hlsl_ir_expr *expr;
     unsigned int i;
@@ -2753,8 +2754,12 @@ static bool lower_float_modulus(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr
         return false;
     hlsl_block_add_instr(block, neg2);

-    if (!(cond = hlsl_add_conditional(ctx, block, ge, arg2, neg2)))
+    operands[0] = ge;
+    operands[1] = arg2;
+    operands[2] = neg2;
+    if (!(cond = hlsl_new_expr(ctx, HLSL_OP3_TERNARY, operands, arg2->data_type, &instr->loc)))
         return false;
+    hlsl_block_add_instr(block, cond);

for (i = 0; i < type->dimx; ++i)
         one_value.u[i].f = 1.0f;
diff --git a/tests/hlsl/arithmetic-float-uniform.shader_test b/tests/hlsl/arithmetic-float-uniform.shader_test
index 708dc2704..f022ac79f 100644
--- a/tests/hlsl/arithmetic-float-uniform.shader_test
+++ b/tests/hlsl/arithmetic-float-uniform.shader_test
@@ -58,6 +58,20 @@ uniform 0 float4 45.0 5.0 0.0 0.0
 draw quad
 probe all rgba (0.0, 0.0, 0.0, 0.0)

+[pixel shader]
+float4 x, y;
+
+float4 main() : sv_target
+{
+    return x % y;
+}
+
+[test]
+uniform 0 float4 5.0 -42.1 4.0 45.0
+uniform 4 float4 15.0 -5.0 4.1 5.0
+draw quad
+probe all rgba (5.0, -2.1, 4.0, 0.0) 4
+
 [require]
 % Infinities are not allowed in SM1
 shader model >= 4.0
diff --git a/tests/hlsl/arithmetic-float.shader_test b/tests/hlsl/arithmetic-float.shader_test
index 558d5d100..73bd627c0 100644
--- a/tests/hlsl/arithmetic-float.shader_test
+++ b/tests/hlsl/arithmetic-float.shader_test
@@ -50,6 +50,19 @@ float4 main() : SV_TARGET
 draw quad
 probe all rgba (0.0, 0.0, 0.0, 0.0)

+[pixel shader]
+float4 main() : sv_target
+{
+    float4 x = {5.0, -42.1, 4.0, 45.0};
+    float4 y = {15.0, -5.0, 4.1, 5.0};
+
+    return x % y;
+}
+
+[test]
+draw quad
+probe all rgba (5.0, -2.1, 4.0, 0.0) 4
+
 [require]
 % Infinities are not allowed in SM1
 shader model >= 4.0

[Nikolay Sivov (＠nsivov)]

On Mon Sep 11 12:13:02 2023 +0000, Giovanni Mascellani wrote:
> > Shouldn't it all go to uniform.shader_test then if it's using uniforms?
>
> Yeah, the correct form of my test is actually this:
> [pixel shader]
> float4 main() : sv_target
> {
>     float4 x = {5.0, -42.1, 4.0, 45.0};
>     float4 y = {15.0, -5.0, 4.1, 5.0};
>     return x % y;
> }
> [test]
> draw quad
> probe all rgba (5.0, -2.1, 4.0, 0.0) 4
>
> Otherwise it doesn't even compile. And that's supposed to be in
> `arithmetic-float.shader_test`, it doesn't use uniform variables.

Ok, tests updated.

[Zebediah Figura (＠zfigura)]

> I think you misunderstood. What I mean is that explict 'if (x) a=b; else a=c;' might be turned to conditional move with x.xxxx mask, as an optimization. The helper is internal, but I'd rather remove it when it's not useful.

Arguably it's still useful as a convenient shorthand for ternary hlsl_new_expr().

[Nikolay Sivov (＠nsivov)]

On Mon Sep 11 21:50:07 2023 +0000, Zebediah Figura wrote:
> > I think you misunderstood. What I mean is that explict 'if (x) a=b;
>
> else a=c;' might be turned to conditional move with x.xxxx mask, as an
> optimization. The helper is internal, but I'd rather remove it when it's
> not useful.
> Arguably it's still useful as a convenient shorthand for ternary hlsl_new_expr().

Sure, we can do that. Does it have to be in this MR, or separately? I think it works either way, I wasn't planning on it, but I can amend with int handling changes, together with corresponding tests.

[Giovanni Mascellani (＠giomasce)]

On Tue Sep 12 13:26:04 2023 +0000, Nikolay Sivov wrote:
> Sure, we can do that. Does it have to be in this MR, or separately? I
> think it works either way, I wasn't planning on it, but I can amend with
> int handling changes, together with corresponding tests.

My favourite way forward would be, in this MR, to change `hlsl_add_conditional()` so that it generates a `TERNARY` instead of an `if`/`else` block. In this way all the three sites currently calling it are fixed at once and without (other) changes.

[Nikolay Sivov (＠nsivov)]

On Tue Sep 12 14:36:29 2023 +0000, Giovanni Mascellani wrote:
> My favourite way forward would be, in this MR, to change
> `hlsl_add_conditional()` so that it generates a `TERNARY` instead of an
> `if`/`else` block. In this way all the three sites currently calling it
> are fixed at once and without (other) changes.

Ok, I'll do that, let's see what happens.