[PATCH v2 0/1] MR336: vkd3d-shader/hlsl: Fix float modulus for vector arguments.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- v2: vkd3d-shader/hlsl: Fix float modulus for vector arguments. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- 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 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336
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 ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44878
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: ```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 ``` 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?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44880
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`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44881
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44882
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? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44883
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/336#note_44884
participants (3)
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)