[PATCH v3 0/5] MR463: hlsl: Fix some type handling for ternaries.

[Zebediah Figura (＠zfigura)]

--
  v3: vkd3d-shader/hlsl: Allow non-numeric types in the ternary operator.
      vkd3d-shader/hlsl: Separate an add_ternary() helper.
      tests: Add many more tests for ternary expressions.

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

[PATCH v3 1/5] vkd3d-shader/hlsl: Remove a redundant definition of the "float" type.

[Zebediah Figura]

From: Zebediah Figura

---
 libs/vkd3d-shader/hlsl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 501bff8cf..b95d2d592 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -3344,7 +3344,6 @@ static void declare_predefined_types(struct hlsl_ctx *ctx)
     effect_types[] =
     {
         {"dword",           HLSL_CLASS_SCALAR, HLSL_TYPE_UINT,          1, 1},
-        {"float",           HLSL_CLASS_SCALAR, HLSL_TYPE_FLOAT,         1, 1},
         {"vector",          HLSL_CLASS_VECTOR, HLSL_TYPE_FLOAT,         4, 1},
         {"matrix",          HLSL_CLASS_MATRIX, HLSL_TYPE_FLOAT,         4, 4},
         {"STRING",          HLSL_CLASS_OBJECT, HLSL_TYPE_STRING,        1, 1},

[PATCH v3 2/5] vkd3d-shader/hlsl: Do not try to lower ternaries of types other than scalar or vector.

[Zebediah Figura]

From: Zebediah Figura

---
 libs/vkd3d-shader/hlsl_codegen.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 5a70878bc..c5049d9f0 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -2764,6 +2764,12 @@ static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, stru
     first = expr->operands[1].node;
     second = expr->operands[2].node;

+    if (cond->data_type->class > HLSL_CLASS_VECTOR || instr->data_type->class > HLSL_CLASS_VECTOR)
+    {
+        hlsl_fixme(ctx, &instr->loc, "Lower ternary of type other than scalar or vector.\n");
+        return false;
+    }
+
     if (ctx->profile->major_version < 4 && ctx->profile->type == VKD3D_SHADER_TYPE_PIXEL)
     {
         struct hlsl_ir_node *abs, *neg;

[PATCH v3 3/5] tests: Add many more tests for ternary expressions.

[Zebediah Figura]

From: Zebediah Figura

---
 Makefile.am                        |   1 +
 tests/hlsl/sm6-ternary.shader_test |  46 +++++
 tests/hlsl/ternary.shader_test     | 275 +++++++++++++++++++++++++++--
 3 files changed, 307 insertions(+), 15 deletions(-)
 create mode 100644 tests/hlsl/sm6-ternary.shader_test

diff --git a/Makefile.am b/Makefile.am
index 2666194a6..fa5afe7a8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -161,6 +161,7 @@ vkd3d_shader_tests = \
    tests/hlsl/side-effects.shader_test \
    tests/hlsl/sign.shader_test \
    tests/hlsl/single-numeric-initializer.shader_test \
+	tests/hlsl/sm6-ternary.shader_test \
    tests/hlsl/smoothstep.shader_test \
    tests/hlsl/sqrt.shader_test \
    tests/hlsl/state-block-syntax.shader_test \
diff --git a/tests/hlsl/sm6-ternary.shader_test b/tests/hlsl/sm6-ternary.shader_test
new file mode 100644
index 000000000..349d32676
--- /dev/null
+++ b/tests/hlsl/sm6-ternary.shader_test
@@ -0,0 +1,46 @@
+% The ternary operator works differently in sm6.
+% It now shortcuts, and is no longer per-component.
+
+[require]
+shader model >= 6.0
+
+[pixel shader]
+uniform float4 x;
+
+float4 main() : sv_target
+{
+    return x.x ? x : x - 1;
+}
+
+[test]
+uniform 0 float4 2.0 3.0 4.0 5.0
+todo draw quad
+probe all rgba (2.0, 3.0, 4.0, 5.0)
+uniform 0 float4 0.0 10.0 11.0 12.0
+todo draw quad
+probe all rgba (-1.0, 9.0, 10.0, 11.0)
+
+
+[pixel shader]
+float4 f;
+
+float4 main() : sv_target
+{
+    float f1 = 0.1, f2 = 0.2, f3;
+    f3 = f.x ? (f1 = 0.5) + 0.2 : (f2 = 0.6);
+    return float4(f1, f2, f3, 0.0);
+}
+
+[test]
+uniform 0 float4 1.0 0.0 0.0 0.0
+draw quad
+probe all rgba (0.5, 0.2, 0.7, 0.0)
+
+
+[pixel shader fail]
+float4 x, y, z;
+
+float4 main() : sv_target
+{
+    return x ? y : z;
+}
diff --git a/tests/hlsl/ternary.shader_test b/tests/hlsl/ternary.shader_test
index 7b4c71f7b..c50920212 100644
--- a/tests/hlsl/ternary.shader_test
+++ b/tests/hlsl/ternary.shader_test
@@ -1,3 +1,8 @@
+[require]
+% The ternary operator works differently in sm6. See sm6-ternary.shader_test.
+shader model < 6.0
+
+
 [pixel shader]
 uniform float4 x;

@@ -8,12 +13,13 @@ float4 main() : sv_target

[test]
 uniform 0 float4 2.0 3.0 4.0 5.0
-todo(sm>=6) draw quad
+draw quad
 probe all rgba (2.0, 3.0, 4.0, 5.0)
 uniform 0 float4 0.0 10.0 11.0 12.0
-todo(sm>=6) draw quad
+draw quad
 probe all rgba (-1.0, 9.0, 10.0, 11.0)

+
 [pixel shader]
 uniform float4 x;

@@ -32,6 +38,7 @@ uniform 0 float4 1.1 3.0 4.0 5.0
 draw quad
 probe all rgba (1.1, 2.0, 0.0, 0.0)

+
 [pixel shader]
 float4 f;

@@ -42,37 +49,275 @@ float4 main() : sv_target
     return float4(f1, f2, f3, 0.0);
 }

-[require]
-shader model < 6.0

[test]
 uniform 0 float4 1.0 0.0 0.0 0.0
 draw quad
 probe all rgba (0.5, 0.6, 0.7, 0.0)

-[require]
-shader model >= 6.0
+
+[pixel shader]
+float4 x, y, z;
+
+float4 main() : sv_target
+{
+    return x ? y : z;
+}

[test]
-uniform 0 float4 1.0 0.0 0.0 0.0
+uniform 0 float4 0.0 1.0 0.0 -3.0
+uniform 4 float4 1.0 2.0 3.0 4.0
+uniform 8 float4 5.0 6.0 7.0 8.0
 draw quad
-probe all rgba (0.5, 0.2, 0.7, 0.0)
+probe all rgba (5.0, 2.0, 7.0, 4.0)

-[require]
-% reset requirements

+% The usual type conversion is applied to the first and second expression, as
+% long as they are numeric.

-[pixel shader fail(sm>=6)]
-float4 x, y, z;
+[pixel shader fail]
+
+uniform float2x4 a;
+uniform float3x2 b;

float4 main() : sv_target
 {
-    return x ? y : z;
+    0 ? a : b;
+    return 0;
+}
+
+
+[pixel shader]
+
+uniform float3x3 a;
+uniform float2x2 b;
+
+float4 main() : sv_target
+{
+    return float4(0 ? a : b);
+}
+
+
+[pixel shader]
+
+uniform float3 a;
+uniform float4 b;
+
+float4 main() : sv_target
+{
+    return float4(0 ? a : b, 1.0);
+}
+
+
+% The condition is more restrictive. Either:
+% * the class and dimensions must match exactly;
+% * one of the two is scalar;
+% * one is a typeN and the other is a type1xN
+
+[pixel shader fail todo]
+
+uniform float4 cond;
+uniform float2x2 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
+}
+
+
+[pixel shader fail todo]
+
+uniform float2x2 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
+}
+
+
+[pixel shader]
+
+uniform float2x2 cond;
+uniform float2x2 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
+}
+
+
+[pixel shader fail todo]
+
+uniform float3 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    (cond ? a : b);
+    return 0;
+}
+
+
+[pixel shader fail todo]
+
+uniform float4 cond;
+uniform float4x1 a, b;
+
+float4 main() : sv_target
+{
+    (cond ? a : b);
+    return 0;
+}
+
+
+% As may be expected, this yields the type of the arguments, not the conditional.
+
+[pixel shader]
+
+uniform float4 cond;
+uniform float1x4 a, b;
+
+float4 main() : sv_target
+{
+    return (cond ? a : b)[0];
+}
+
+
+[pixel shader todo]
+
+uniform float1x4 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return (cond ? a : b)[0];
+}
+
+
+[pixel shader fail]
+
+uniform float1x4 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return (cond ? a : b)[0][0];
+}
+
+
+[pixel shader]
+
+uniform float1 cond;
+uniform float1x1 a, b;
+
+float4 main() : sv_target
+{
+    return (cond ? a : b)[0][0];
+}
+
+
+[pixel shader]
+
+uniform float cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
 }

[test]
-uniform 0 float4 0.0 1.0 0.0 -3.0
+uniform 0 float4 1.0 0.0 0.0 0.0
 uniform 4 float4 1.0 2.0 3.0 4.0
 uniform 8 float4 5.0 6.0 7.0 8.0
 draw quad
-probe all rgba (5.0, 2.0, 7.0, 4.0)
+probe all rgba (1.0, 2.0, 3.0, 4.0)
+
+
+[pixel shader todo]
+
+// "scalar" can mean any 1-component numeric type.
+uniform float1x1 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
+}
+
+
+[pixel shader todo]
+
+uniform float4 cond;
+uniform float4 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a.x : b.x);
+}
+
+[test]
+uniform 0 float4 1.0 0.0 1.0 0.0
+uniform 4 float4 1.0 2.0 3.0 4.0
+uniform 8 float4 5.0 6.0 7.0 8.0
+todo draw quad
+probe all rgba (1.0, 5.0, 1.0, 5.0)
+
+
+[pixel shader todo]
+
+// "scalar" can mean any 1-component numeric type.
+uniform float4 cond;
+uniform float1x1 a, b;
+
+float4 main() : sv_target
+{
+    return float4(cond ? a : b);
+}
+
+
+% Objects can be used, but their types have to be identical.
+
+[pixel shader todo]
+Texture2D t;
+
+float4 main() : sv_target
+{
+    Texture2D tmp = 0 ? t : t;
+    return 0;
+}
+
+
+[pixel shader fail]
+Texture2D t;
+float4 f;
+
+float4 main() : sv_target
+{
+    f ? t : t;
+    return 0;
+}
+
+
+[pixel shader fail]
+Texture2D t;
+Texture3D u;
+
+float4 main() : sv_target
+{
+    0 ? t : u;
+    return 0;
+}
+
+
+% Of course objects cannot be used as the condition.
+
+[pixel shader fail todo]
+Texture2D t;
+
+float4 main() : sv_target
+{
+    t ? 0 : 1;
+    return 0;
+}

[PATCH v3 4/5] vkd3d-shader/hlsl: Separate an add_ternary() helper.

[Zebediah Figura]

From: Zebediah Figura

---
 libs/vkd3d-shader/hlsl.y | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 5e0d48d3c..7927af592 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -3998,6 +3998,27 @@ static struct hlsl_block *add_constructor(struct hlsl_ctx *ctx, struct hlsl_type
     return params->instrs;
 }

+static bool add_ternary(struct hlsl_ctx *ctx, struct hlsl_block *block,
+        struct hlsl_ir_node *cond, struct hlsl_ir_node *first, struct hlsl_ir_node *second)
+{
+    struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {0};
+    struct hlsl_type *common_type;
+
+    if (!(common_type = get_common_numeric_type(ctx, first, second, &first->loc)))
+        return false;
+
+    if (!(first = add_implicit_conversion(ctx, block, first, common_type, &first->loc)))
+        return false;
+
+    if (!(second = add_implicit_conversion(ctx, block, second, common_type, &second->loc)))
+        return false;
+
+    args[0] = cond;
+    args[1] = first;
+    args[2] = second;
+    return add_expr(ctx, block, HLSL_OP3_TERNARY, args, common_type, &first->loc);
+}
+
 static unsigned int hlsl_offset_dim_count(enum hlsl_sampler_dim dim)
 {
     switch (dim)
@@ -7061,27 +7082,13 @@ conditional_expr:
             struct hlsl_ir_node *cond = node_from_block($1);
             struct hlsl_ir_node *first = node_from_block($3);
             struct hlsl_ir_node *second = node_from_block($5);
-            struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = { 0 };
-            struct hlsl_type *common_type;

hlsl_block_add_block($1, $3);
             hlsl_block_add_block($1, $5);
             destroy_block($3);
             destroy_block($5);

-            if (!(common_type = get_common_numeric_type(ctx, first, second, &@3)))
-                YYABORT;
-
-            if (!(first = add_implicit_conversion(ctx, $1, first, common_type, &@3)))
-                YYABORT;
-
-            if (!(second = add_implicit_conversion(ctx, $1, second, common_type, &@5)))
-                YYABORT;
-
-            args[0] = cond;
-            args[1] = first;
-            args[2] = second;
-            if (!add_expr(ctx, $1, HLSL_OP3_TERNARY, args, common_type, &@1))
+            if (!add_ternary(ctx, $1, cond, first, second))
                 YYABORT;
             $$ = $1;
         }

[PATCH v3 5/5] vkd3d-shader/hlsl: Allow non-numeric types in the ternary operator.

[Zebediah Figura]

From: Zebediah Figura

---
 libs/vkd3d-shader/hlsl.y       | 34 ++++++++++++++++++++++++++++------
 tests/hlsl/ternary.shader_test |  2 +-
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 7927af592..13275e796 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -4004,14 +4004,36 @@ static bool add_ternary(struct hlsl_ctx *ctx, struct hlsl_block *block,
     struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = {0};
     struct hlsl_type *common_type;

-    if (!(common_type = get_common_numeric_type(ctx, first, second, &first->loc)))
-        return false;
+    if (first->data_type->class <= HLSL_CLASS_LAST_NUMERIC
+            && second->data_type->class <= HLSL_CLASS_LAST_NUMERIC)
+    {
+        if (!(common_type = get_common_numeric_type(ctx, first, second, &first->loc)))
+            return false;

-    if (!(first = add_implicit_conversion(ctx, block, first, common_type, &first->loc)))
-        return false;
+        if (!(first = add_implicit_conversion(ctx, block, first, common_type, &first->loc)))
+            return false;

-    if (!(second = add_implicit_conversion(ctx, block, second, common_type, &second->loc)))
-        return false;
+        if (!(second = add_implicit_conversion(ctx, block, second, common_type, &second->loc)))
+            return false;
+    }
+    else
+    {
+        struct vkd3d_string_buffer *first_string, *second_string;
+
+        if (!hlsl_types_are_equal(first->data_type, second->data_type))
+        {
+            first_string = hlsl_type_to_string(ctx, first->data_type);
+            second_string = hlsl_type_to_string(ctx, second->data_type);
+            if (first_string && second_string)
+                hlsl_error(ctx, &first->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
+                        "Ternary argument types '%s' and '%s' do not match.",
+                        first_string->buffer, second_string->buffer);
+            hlsl_release_string_buffer(ctx, first_string);
+            hlsl_release_string_buffer(ctx, second_string);
+        }
+
+        common_type = first->data_type;
+    }

args[0] = cond;
     args[1] = first;
diff --git a/tests/hlsl/ternary.shader_test b/tests/hlsl/ternary.shader_test
index c50920212..f680468c4 100644
--- a/tests/hlsl/ternary.shader_test
+++ b/tests/hlsl/ternary.shader_test
@@ -289,7 +289,7 @@ float4 main() : sv_target
 }

-[pixel shader fail]
+[pixel shader fail todo]
 Texture2D t;
 float4 f;

[Zebediah Figura (＠zfigura)]

v2: Move the sm6 tests to a separate file; the ternary operator works completely differently in sm6.

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl/ternary.shader_test:
> {
> - return x ? y : z;
> - 0 ? a : b;
> - return 0;
>
> +}
> -
> -
>
> +[pixel shader]
> -
>
> +uniform float3x3 a;
> +uniform float2x2 b;
> -
>
> +float4 main() : sv_target
> +{
> - return float4(0 ? a : b);
>
> +}

Could you also add a quick `[test]` for expected-successful shaders? Semantics should be obvious here, but I'm always braced for surprises...

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl/ternary.shader_test:
> -
>
> +[pixel shader]
> -
>
> +uniform float3 a;
> +uniform float4 b;
> -
>
> +float4 main() : sv_target
> +{
> - return float4(0 ? a : b, 1.0);
>
> +}
> -
> -
>
> +% The condition is more restrictive. Either:
> +% * the class and dimensions must match exactly;
> +% * one of the two is scalar;
> +% * one is a typeN and the other is a type1xN

This is a bit hard to parse for me:
* "Condition" is overloaded here: one meaning if the condition (i.e., first) argument to the ternary construct, which I believe to be what you mean; another meaning is the fact that the following is a list of "conditions" that must be satisfied for compilation to succeed;
* you mention "two" things, and then "one" and the "other": since the ternary construct deals with three arguments, I'm not sure of what you mean. Are the "two" the type of the first argument and the combined type of the latest two arguments to the ternary construct?

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl/ternary.shader_test:
> -
>
> +float4 main() : sv_target
> +{
> - (cond ? a : b);
> - return 0;
>
> +}
> -
> -
>
> +[pixel shader fail todo]
> -
>
> +uniform float4 cond;
> +uniform float4x1 a, b;
> -
>
> +float4 main() : sv_target
> +{
> - (cond ? a : b);

Is there a reason for the parentheses here?

[Zebediah Figura (＠zfigura)]

On Wed Dec 13 10:12:51 2023 +0000, Giovanni Mascellani wrote:
> This is a bit hard to parse for me:
> - "Condition" is overloaded here: one meaning if the condition (i.e.,
>
> first) argument to the ternary construct, which I believe to be what you
> mean; another meaning is the fact that the following is a list of
> "conditions" that must be satisfied for compilation to succeed;
> - you mention "two" things, and then "one" and the "other": since the
>
> ternary construct deals with three arguments, I'm not sure of what you
> mean. Are the "two" the type of the first argument and the combined type
> of the latest two arguments to the ternary construct?

I'll try to clarify this one. Your assumptions of my meaning are correct.

[Zebediah Figura (＠zfigura)]

On Wed Dec 13 10:12:52 2023 +0000, Giovanni Mascellani wrote:
> Is there a reason for the parentheses here?

Personal style preference. I usually parenthesize ternaries in realistic cases as well.

[Zebediah Figura (＠zfigura)]

On Wed Dec 13 10:12:50 2023 +0000, Giovanni Mascellani wrote:
> Could you also add a quick `[test]` for expected-successful shaders?
> Semantics should be obvious here, but I'm always braced for surprises...

If you feel strongly about it, I guess, but the point of most of these tests is to figure out what is the HLSL type of the result. I don't see what a [test] would demonstrate here, and it'd add some clutter.

[Giovanni Mascellani (＠giomasce)]

On Wed Dec 13 21:54:04 2023 +0000, Zebediah Figura wrote:
> If you feel strongly about it, I guess, but the point of most of these
> tests is to figure out what is the HLSL type of the result. I don't see
> what a [test] would demonstrate here, and it'd add some clutter.

Mostly that I'm suspicious about surprises by the native compiler, and in my view that doesn't feel too much clutter. However, no, I can't say I really feel that strong here.