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

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov

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

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

[PATCH v2 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.shader_test | 14 ++++++++++++++
 2 files changed, 20 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.shader_test b/tests/hlsl/arithmetic-float.shader_test
index 558d5d100..529f9cd98 100644
--- a/tests/hlsl/arithmetic-float.shader_test
+++ b/tests/hlsl/arithmetic-float.shader_test
@@ -50,6 +50,20 @@ float4 main() : SV_TARGET
 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

[Giovanni Mascellani (＠giomasce)]

Given that `hlsl_add_conditional()` is also used in `lower_int_division()` and `lower_int_modulus()`, which likely have the same problem, shouldn't we rather fix `hlsl_add_conditional()` instead?

Also, please add the corresponding test in `arithmetic-float-uniform.shader_test` too:
```c
[pixel shader]
float4 x = {5.0, -42.1, 4.0, 45.0};
float4 y = {15.0, -5.0, 4.1, 5.0};

float4 main() : sv_target
{
    return x % y;
}

[test]
draw quad
probe all rgba (5.0, -2.1, 4.0, 0.0) 4
```

[Nikolay Sivov (＠nsivov)]

On Mon Sep 11 11:27:36 2023 +0000, Giovanni Mascellani wrote:
> Given that `hlsl_add_conditional()` is also used in
> `lower_int_division()` and `lower_int_modulus()`, which likely have the
> same problem, shouldn't we rather fix `hlsl_add_conditional()` instead?
> Also, please add the corresponding test in
> `arithmetic-float-uniform.shader_test` too:
> [pixel shader]
> float4 x = {5.0, -42.1, 4.0, 45.0};
> float4 y = {15.0, -5.0, 4.1, 5.0};
> float4 main() : sv_target
> {
>     return x % y;
> }
> [test]
> draw quad
> probe all rgba (5.0, -2.1, 4.0, 0.0) 4

I don't think we should, because it does not mean the same thing. In the future we might want to turn certain if/else blocks to conditional moves, I don't know if that happens on Windows. Regarding this test, it only differs in a way the data is initialized, why is it important?

[Giovanni Mascellani (＠giomasce)]

On Mon Sep 11 11:27:35 2023 +0000, Nikolay Sivov wrote:
> I don't think we should, because it does not mean the same thing. In the
> future we might want to turn certain if/else blocks to conditional
> moves, I don't know if that happens on Windows. Regarding this test, it
> only differs in a way the data is initialized, why is it important?

That's just an internal helper for us, it doesn't matter what happens on Windows or will happen in the future.

WRT the test, the general philosophy is to keep `arithmetic-float.shader_test` and `arithmetic-float-uniform.shader_test` synchronized, so that the correctness of the constant propagation pass is checked too. Actually, I got it backward: the test you wrote should go into `arithmetic-float-uniform.shader_test`, and the test I wrote should go to `arithmetic-float.shader_test`.

[Nikolay Sivov (＠nsivov)]

> That's just an internal helper for us, it doesn't matter what happens on Windows or will happen in the future.

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.

[Nikolay Sivov (＠nsivov)]

> WRT the test, the general philosophy is to keep `arithmetic-float.shader_test` and `arithmetic-float-uniform.shader_test` synchronized, so that the correctness of the constant propagation pass is checked too. Actually, I got it backward: the test you wrote should go into `arithmetic-float-uniform.shader_test`, and the test I wrote should go to `arithmetic-float.shader_test`.

Shouldn't it all go to uniform.shader_test then if it's using uniforms?

[Giovanni Mascellani (＠giomasce)]

> 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:
```c
[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.