[PATCH 0/1] MR669: vkd3d-shader/hlsl: Implement ternary operator for older vertex profiles.

[Nikolay Sivov (＠nsivov)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56333
Signed-off-by: Nikolay Sivov

[PATCH 1/1] vkd3d-shader/hlsl: Implement ternary operator for older vertex profiles.

[Nikolay Sivov]

From: Nikolay Sivov

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56333
Signed-off-by: Nikolay Sivov 
---
 libs/vkd3d-shader/d3dbc.c        |  4 +++
 libs/vkd3d-shader/hlsl.c         |  1 +
 libs/vkd3d-shader/hlsl.h         |  1 +
 libs/vkd3d-shader/hlsl_codegen.c | 42 +++++++++++++++++++++++++++++---
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c
index 27f5c8104..a697a2a80 100644
--- a/libs/vkd3d-shader/d3dbc.c
+++ b/libs/vkd3d-shader/d3dbc.c
@@ -2329,6 +2329,10 @@ static void write_sm1_expr(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *b
             }
             break;

+        case HLSL_OP2_SLT:
+            write_sm1_binary_op(ctx, buffer, D3DSIO_SLT, &instr->reg, &arg1->reg, &arg2->reg);
+            break;
+
         case HLSL_OP3_CMP:
             write_sm1_ternary_op(ctx, buffer, D3DSIO_CMP, &instr->reg, &arg1->reg, &arg2->reg, &arg3->reg);
             break;
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 353e34d21..eb766b4d9 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -2610,6 +2610,7 @@ const char *debug_hlsl_expr_op(enum hlsl_ir_expr_op op)
         [HLSL_OP2_MUL]         = "*",
         [HLSL_OP2_NEQUAL]      = "!=",
         [HLSL_OP2_RSHIFT]      = ">>",
+        [HLSL_OP2_SLT]         = "slt",

[HLSL_OP3_CMP]         = "cmp",
         [HLSL_OP3_DP2ADD]      = "dp2add",
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 5b73857b9..f6cf2fe4a 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -591,6 +591,7 @@ enum hlsl_ir_expr_op
     HLSL_OP2_MUL,
     HLSL_OP2_NEQUAL,
     HLSL_OP2_RSHIFT,
+    HLSL_OP2_SLT,

/* DP2ADD(a, b, c) computes the scalar product of a.xy and b.xy,
      * then adds c. */
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 307f86f55..e7fc02d05 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -2903,7 +2903,7 @@ static bool lower_floor(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, struct
     return true;
 }

-/* Use 'movc' for the ternary operator. */
+/* Use movc/cmp/slt for the ternary operator. */
 static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, struct hlsl_block *block)
 {
     struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS] = { 0 }, *replacement;
@@ -2949,8 +2949,44 @@ static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, stru
     }
     else if (ctx->profile->major_version < 4 && ctx->profile->type == VKD3D_SHADER_TYPE_VERTEX)
     {
-        hlsl_fixme(ctx, &instr->loc, "Ternary operator is not implemented for %s profile.", ctx->profile->name);
-        return false;
+        struct hlsl_ir_node *neg, *slt, *sum, *mul, *cond2;
+
+        /* Expression used here is "slt(<cond>) * (first - second) + second". */
+
+        if (ctx->profile->major_version == 3)
+        {
+            if (!(cond2 = hlsl_new_unary_expr(ctx, HLSL_OP1_ABS, cond, &instr->loc)))
+                return false;
+        }
+        else
+        {
+            if (!(cond2 = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, cond, cond)))
+                return false;
+        }
+        hlsl_block_add_instr(block, cond2);
+
+        if (!(neg = hlsl_new_unary_expr(ctx, HLSL_OP1_NEG, cond2, &instr->loc)))
+            return false;
+        hlsl_block_add_instr(block, neg);
+
+        if (!(slt = hlsl_new_binary_expr(ctx, HLSL_OP2_SLT, neg, cond2)))
+            return false;
+        hlsl_block_add_instr(block, slt);
+
+        if (!(neg = hlsl_new_unary_expr(ctx, HLSL_OP1_NEG, second, &instr->loc)))
+            return false;
+        hlsl_block_add_instr(block, neg);
+
+        if (!(sum = hlsl_new_binary_expr(ctx, HLSL_OP2_ADD, first, neg)))
+            return false;
+        hlsl_block_add_instr(block, sum);
+
+        if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, slt, sum)))
+            return false;
+        hlsl_block_add_instr(block, mul);
+
+        if (!(replacement = hlsl_new_binary_expr(ctx, HLSL_OP2_ADD, mul, second)))
+            return false;
     }
     else
     {

[Zebediah Figura (＠zfigura)]

"slt" is only valid for vertex shaders.

We've been rather lax about this so far, which isn't great. But we definitely shouldn't be using VS-only instructions in a pixel shader.

[Nikolay Sivov (＠nsivov)]

On Thu Feb 22 20:10:06 2024 +0000, Zebediah Figura wrote:
> "slt" is only valid for vertex shaders.
> We've been rather lax about this so far, which isn't great. But we
> definitely shouldn't be using VS-only instructions in a pixel shader.

Yes, exactly. It's used in place of 'cmp'. There is a profile type check, it's not used for pixel shaders.

[Zebediah Figura (＠zfigura)]

On Thu Feb 22 20:10:06 2024 +0000, Nikolay Sivov wrote:
> Yes, exactly. It's used in place of 'cmp'. There is a profile type
> check, it's not used for pixel shaders.

Ugh, I'm sorry, I really need to read instead of blindly assuming things.

[PATCH 0/1] MR669: vkd3d-shader/hlsl: Implement ternary operator for older vertex profiles. - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.

[PATCH 0/1] MR669: vkd3d-shader/hlsl: Implement ternary operator for older vertex profiles. - approved

[Giovanni Mascellani (＠giomasce)]

This merge request was approved by Giovanni Mascellani.

[Giovanni Mascellani (＠giomasce)]

A couple of months ago [there was a request on wine-devel(https://www.winehq.org/mailman3/hyperkitty/list/[e-mail]/thread...) to make it easier for testers to have a list of the applications that it's sensible to test in view of a vkd3d release. No definitive decision was taken, but the request makes sense to me and I wrote a proposal. Nothing happened since then, but since this commit is specifically for fixing an application bug it might be a good opportunity to try to set a good practice. Would you consider adding something like this to your commit message?
```
Impacted-Application: MotorM4X
```

[Nikolay Sivov (＠nsivov)]

On Fri Feb 23 11:55:28 2024 +0000, Giovanni Mascellani wrote:
> A couple of months ago [there was a request on
> wine-devel(https://www.winehq.org/mailman3/hyperkitty/list/[e-mail]/thread...)
> to make it easier for testers to have a list of the applications that
> it's sensible to test in view of a vkd3d release. No definitive decision
> was taken, but the request makes sense to me and I wrote a proposal.
> Nothing happened since then, but since this commit is specifically for
> fixing an application bug it might be a good opportunity to try to set a
> good practice. Would you consider adding something like this to your
> commit message?
> Impacted-Application: MotorM4X

I think a bug link gives more information.

[Giovanni Mascellani (＠giomasce)]

On Fri Feb 23 11:55:27 2024 +0000, Nikolay Sivov wrote:
> I think a bug link gives more information.

But it makes it harder to find the interesting information for the tester and it's not easy to grep. They're not mutually exclusive.

[Gijs Vermeulen (＠gverm)]

On Fri Feb 23 12:04:21 2024 +0000, Giovanni Mascellani wrote:
> But it makes it harder to find the interesting information for the
> tester and it's not easy to grep. They're not mutually exclusive.

@giomasce Maybe these things could just be saved somewhere for release notes, even if that's outside the repo. As long as these things are visible when a new release is made, that should be good enough I think. IMHO it doesn't seem like a good idea to be putting these things inside commit messages.

[Giovanni Mascellani (＠giomasce)]

On Fri Feb 23 13:24:45 2024 +0000, Gijs Vermeulen wrote:
> @giomasce Maybe these things could just be saved somewhere for release
> notes, even if that's outside the repo. As long as these things are
> visible when a new release is made, that should be good enough I think.
> IMHO it doesn't seem like a good idea to be putting these things inside
> commit messages.

The advantage of commit messages is that they already exist and require no additional maintenance. I am not aware of other places with the same features. Also commit messages allow to link an application with potentially problematic commits.

[PATCH 0/1] MR669: vkd3d-shader/hlsl: Implement ternary operator for older vertex profiles. - approved

[Henri Verbeet (＠hverbeet)]

This merge request was approved by Henri Verbeet.