[PATCH vkd3d 1/5] tests: Add some simple tests for HLSL function calls.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 Makefile.am                     |   2 +
 tests/hlsl-function.shader_test | 166 ++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+)
 create mode 100644 tests/hlsl-function.shader_test

diff --git a/Makefile.am b/Makefile.am
index 491537f6..dd5966ee 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -65,6 +65,7 @@ vkd3d_shader_tests = \
 	tests/hlsl-cross.shader_test \
 	tests/hlsl-duplicate-modifiers.shader_test \
 	tests/hlsl-for.shader_test \
+	tests/hlsl-function.shader_test \
 	tests/hlsl-function-overload.shader_test \
 	tests/hlsl-gather-offset.shader_test \
 	tests/hlsl-gather.shader_test \
@@ -316,6 +317,7 @@ XFAIL_TESTS = \
 	tests/hlsl-bool-cast.shader_test \
 	tests/hlsl-duplicate-modifiers.shader_test \
 	tests/hlsl-for.shader_test \
+	tests/hlsl-function.shader_test \
 	tests/hlsl-function-overload.shader_test \
 	tests/hlsl-gather.shader_test \
 	tests/hlsl-intrinsic-override.shader_test \
diff --git a/tests/hlsl-function.shader_test b/tests/hlsl-function.shader_test
new file mode 100644
index 00000000..af254fb3
--- /dev/null
+++ b/tests/hlsl-function.shader_test
@@ -0,0 +1,166 @@
+[pixel shader fail]
+
+float4 func();
+
+float4 main() : sv_target
+{
+    return func();
+}
+
+% It's legal to call an undefined function in unused code, though.
+
+[pixel shader]
+
+float4 func();
+
+float4 unused()
+{
+    return func();
+}
+
+float4 main() : sv_target
+{
+    return 0;
+}
+
+[pixel shader fail]
+
+void func(inout float o)
+{
+    o += 0.1;
+}
+
+float4 main() : sv_target
+{
+    float x = 0;
+    func(x + 1);
+    return 0;
+}
+
+[pixel shader fail]
+
+void func(inout float2 o)
+{
+    o += 0.1;
+}
+
+float4 main() : sv_target
+{
+    float2 x = 0;
+    func(x.yy);
+    return 0;
+}
+
+[pixel shader fail]
+
+void func(out float o)
+{
+    o = 0.1;
+}
+
+float4 main() : sv_target
+{
+    const float x = 0;
+    func(x);
+    return x;
+}
+
+[pixel shader fail]
+
+void func(inout float o)
+{
+}
+
+float4 main() : sv_target
+{
+    const float x = 0;
+    func(x);
+    return x;
+}
+
+[pixel shader fail]
+
+void func()
+{
+}
+
+float4 main() : sv_target
+{
+    return func();
+}
+
+[pixel shader fail]
+
+% Returning a void expression from a void function is legal in C, but not in HLSL.
+
+void foo()
+{
+}
+
+void bar()
+{
+    return foo();
+}
+
+float4 main() : sv_target
+{
+    bar();
+    return 0;
+}
+
+[pixel shader fail]
+
+float4 main() : sv_target
+{
+    func();
+    return 0;
+}
+
+void func()
+{
+}
+
+[pixel shader]
+
+float func(in float a, out float b, inout float c)
+{
+    c -= 0.2;
+    b = a * 2;
+    return a + 0.2;
+}
+
+float4 main() : sv_target
+{
+    float x[2], ret;
+
+    x[0] = 0.1;
+    x[1] = 0.9;
+    ret = func(0.3, x[0], x[1]);
+
+    return float4(ret, x[0], x[1], 0);
+}
+
+[test]
+draw quad
+probe all rgba (0.5, 0.6, 0.7, 0)
+
+[pixel shader]
+
+void func(in float a, inout float2 b)
+{
+    b.y += 0.1;
+    b *= a;
+}
+
+float4 main() : sv_target
+{
+    float3 q = float3(0.1, 0.2, 0.3);
+
+    func(3.0, q.zx);
+    func(0.5, q.yz);
+    return float4(q, 0);
+}
+
+[test]
+draw quad
+probe all rgba (0.6, 0.1, 0.5, 0)
-- 
2.34.1

[PATCH vkd3d 2/5] tests: Add some tests for casting output arguments to functions.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 Makefile.am                          |   2 +
 tests/hlsl-function-cast.shader_test | 103 +++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 tests/hlsl-function-cast.shader_test

diff --git a/Makefile.am b/Makefile.am
index dd5966ee..75c5e4b1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -66,6 +66,7 @@ vkd3d_shader_tests = \
 	tests/hlsl-duplicate-modifiers.shader_test \
 	tests/hlsl-for.shader_test \
 	tests/hlsl-function.shader_test \
+	tests/hlsl-function-cast.shader_test \
 	tests/hlsl-function-overload.shader_test \
 	tests/hlsl-gather-offset.shader_test \
 	tests/hlsl-gather.shader_test \
@@ -318,6 +319,7 @@ XFAIL_TESTS = \
 	tests/hlsl-duplicate-modifiers.shader_test \
 	tests/hlsl-for.shader_test \
 	tests/hlsl-function.shader_test \
+	tests/hlsl-function-cast.shader_test \
 	tests/hlsl-function-overload.shader_test \
 	tests/hlsl-gather.shader_test \
 	tests/hlsl-intrinsic-override.shader_test \
diff --git a/tests/hlsl-function-cast.shader_test b/tests/hlsl-function-cast.shader_test
new file mode 100644
index 00000000..72ee61eb
--- /dev/null
+++ b/tests/hlsl-function-cast.shader_test
@@ -0,0 +1,103 @@
+% Test implicit and explicit casts on function output parameters.
+
+[pixel shader]
+
+uniform float4 f;
+
+void func(out float4 o)
+{
+    o = f;
+}
+
+float4 main() : sv_target
+{
+    int4 x;
+    func(x);
+    return x;
+}
+
+[test]
+uniform 0 float4 -1.9 -1.0 2.9 4.0
+draw quad
+probe all rgba (-1.0, -1.0, 2.0, 4.0)
+
+% As above, but cast "x" to float4 first.
+
+[pixel shader]
+
+uniform float4 f;
+
+void func(out float4 o)
+{
+    o = f;
+}
+
+float4 main() : sv_target
+{
+    int4 x;
+    func((float4)x);
+    return x;
+}
+
+[test]
+uniform 0 float4 -1.9 -1.0 2.9 4.0
+draw quad
+probe all rgba (-1.0, -1.0, 2.0, 4.0)
+
+% As above, but declare "x" as float4 and cast it to int4.
+
+[pixel shader]
+
+uniform float4 f;
+
+void func(out float4 o)
+{
+    o = f;
+}
+
+float4 main() : sv_target
+{
+    float4 x;
+    func((int4)x);
+    return x;
+}
+
+[test]
+uniform 0 float4 -1.9 -1.0 2.9 4.0
+draw quad
+probe all rgba (-1.0, -1.0, 2.0, 4.0)
+
+[pixel shader]
+
+void func(inout float4 a)
+{
+    a += 0.1;
+}
+
+float4 main(uniform int4 i) : sv_target
+{
+    int4 x = i;
+    func(x);
+    return x;
+}
+
+[test]
+uniform 0 int4 -2 0 1 -3000000
+draw quad
+probe all rgba (-1.0, 0.0, 1.0, -3000000.0)
+
+% You can't cast an inout parameter, though.
+
+[pixel shader fail]
+
+void func(inout float4 a)
+{
+    a += 0.1;
+}
+
+float4 main(uniform int4 i) : sv_target
+{
+    int4 x = i;
+    func((float4)x);
+    return x;
+}
-- 
2.34.1

[PATCH vkd3d 2/5] tests: Add some tests for casting output arguments to functions.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>

Il 16/02/22 06:29, Zebediah Figura ha scritto:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>   Makefile.am                          |   2 +
>   tests/hlsl-function-cast.shader_test | 103 +++++++++++++++++++++++++++
>   2 files changed, 105 insertions(+)
>   create mode 100644 tests/hlsl-function-cast.shader_test
>
> diff --git a/Makefile.am b/Makefile.am
> index dd5966ee..75c5e4b1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -66,6 +66,7 @@ vkd3d_shader_tests = \
>   tests/hlsl-duplicate-modifiers.shader_test \
>   tests/hlsl-for.shader_test \
>   tests/hlsl-function.shader_test \
> +	tests/hlsl-function-cast.shader_test \
>   tests/hlsl-function-overload.shader_test \
>   tests/hlsl-gather-offset.shader_test \
>   tests/hlsl-gather.shader_test \
> @@ -318,6 +319,7 @@ XFAIL_TESTS = \
>   tests/hlsl-duplicate-modifiers.shader_test \
>   tests/hlsl-for.shader_test \
>   tests/hlsl-function.shader_test \
> +	tests/hlsl-function-cast.shader_test \
>   tests/hlsl-function-overload.shader_test \
>   tests/hlsl-gather.shader_test \
>   tests/hlsl-intrinsic-override.shader_test \
> diff --git a/tests/hlsl-function-cast.shader_test b/tests/hlsl-function-cast.shader_test
> new file mode 100644
> index 00000000..72ee61eb
> --- /dev/null
> +++ b/tests/hlsl-function-cast.shader_test
> @@ -0,0 +1,103 @@
> +% Test implicit and explicit casts on function output parameters.
> +
> +[pixel shader]
> +
> +uniform float4 f;
> +
> +void func(out float4 o)
> +{
> +    o = f;
> +}
> +
> +float4 main() : sv_target
> +{
> +    int4 x;
> +    func(x);
> +    return x;
> +}
> +
> +[test]
> +uniform 0 float4 -1.9 -1.0 2.9 4.0
> +draw quad
> +probe all rgba (-1.0, -1.0, 2.0, 4.0)
> +
> +% As above, but cast "x" to float4 first.
> +
> +[pixel shader]
> +
> +uniform float4 f;
> +
> +void func(out float4 o)
> +{
> +    o = f;
> +}
> +
> +float4 main() : sv_target
> +{
> +    int4 x;
> +    func((float4)x);
> +    return x;
> +}
> +
> +[test]
> +uniform 0 float4 -1.9 -1.0 2.9 4.0
> +draw quad
> +probe all rgba (-1.0, -1.0, 2.0, 4.0)
> +
> +% As above, but declare "x" as float4 and cast it to int4.
> +
> +[pixel shader]
> +
> +uniform float4 f;
> +
> +void func(out float4 o)
> +{
> +    o = f;
> +}
> +
> +float4 main() : sv_target
> +{
> +    float4 x;
> +    func((int4)x);
> +    return x;
> +}
> +
> +[test]
> +uniform 0 float4 -1.9 -1.0 2.9 4.0
> +draw quad
> +probe all rgba (-1.0, -1.0, 2.0, 4.0)
> +
> +[pixel shader]
> +
> +void func(inout float4 a)
> +{
> +    a += 0.1;
> +}
> +
> +float4 main(uniform int4 i) : sv_target
> +{
> +    int4 x = i;
> +    func(x);
> +    return x;
> +}
> +
> +[test]
> +uniform 0 int4 -2 0 1 -3000000
> +draw quad
> +probe all rgba (-1.0, 0.0, 1.0, -3000000.0)
> +
> +% You can't cast an inout parameter, though.
> +
> +[pixel shader fail]
> +
> +void func(inout float4 a)
> +{
> +    a += 0.1;
> +}
> +
> +float4 main(uniform int4 i) : sv_target
> +{
> +    int4 x = i;
> +    func((float4)x);
> +    return x;
> +}

[PATCH vkd3d 2/5] tests: Add some tests for casting output arguments to functions.

[Francisco Casas]

Hello,

February 16, 2022 2:47 AM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
> +
> +% You can't cast an inout parameter, though.
> +
> +[pixel shader fail]
> +
> +void func(inout float4 a)
> +{
> + a += 0.1;
> +}
> +
> +float4 main(uniform int4 i) : sv_target
> +{
> + int4 x = i;
> + func((float4)x);
> + return x;
> +}
> -- 
> 2.34.1

For some reason the shader actually compiles using ps_3_0 and lower.
Maybe a [require] should be added? Or maybe, we should just allow it always.

Thanks,
Francisco

[PATCH vkd3d 2/5] tests: Add some tests for casting output arguments to functions.

[Matteo Bruni]

On Thu, Feb 17, 2022 at 5:17 PM Francisco Casas <fcasas(a)codeweavers.com> wrote:
> Hello,
>
> February 16, 2022 2:47 AM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
> > +
> > +% You can't cast an inout parameter, though.
> > +
> > +[pixel shader fail]
> > +
> > +void func(inout float4 a)
> > +{
> > + a += 0.1;
> > +}
> > +
> > +float4 main(uniform int4 i) : sv_target
> > +{
> > + int4 x = i;
> > + func((float4)x);
> > + return x;
> > +}
> > --
> > 2.34.1
>
> For some reason the shader actually compiles using ps_3_0 and lower.
> Maybe a [require] should be added? Or maybe, we should just allow it always.

Nice catch! On SM1, aside from some very specific cases, only floats
really exist. In particular, mathematical expressions only apply to
floats and, indeed, the native compiler generates a float uniform for
'i'. I guess the cast is implicitly dropped when compiling for e.g.
ps_2_0 so there is no error there.

I resent the patch without this test for the time being.

[PATCH vkd3d 3/5] vkd3d-shader/hlsl: Dump only the entry point.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 libs/vkd3d-shader/hlsl_codegen.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index eac3513c..20d4ffe9 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -954,23 +954,6 @@ static unsigned int index_instructions(struct hlsl_block *block, unsigned int in
     return index;
 }
 
-static void dump_function_decl(struct rb_entry *entry, void *context)
-{
-    struct hlsl_ir_function_decl *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function_decl, entry);
-    struct hlsl_ctx *ctx = context;
-
-    if (func->has_body)
-        hlsl_dump_function(ctx, func);
-}
-
-static void dump_function(struct rb_entry *entry, void *context)
-{
-    struct hlsl_ir_function *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry);
-    struct hlsl_ctx *ctx = context;
-
-    rb_for_each_entry(&func->overloads, dump_function_decl, ctx);
-}
-
 /* Compute the earliest and latest liveness for each variable. In the case that
  * a variable is accessed inside of a loop, we promote its liveness to extend
  * to at least the range of the entire loop. Note that we don't need to do this
@@ -1814,7 +1797,7 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
     compute_liveness(ctx, entry_func);
 
     if (TRACE_ON())
-        rb_for_each_entry(&ctx->functions, dump_function, ctx);
+        hlsl_dump_function(ctx, entry_func);
 
     allocate_temp_registers(ctx, entry_func);
     if (ctx->profile->major_version < 4)
-- 
2.34.1

[PATCH vkd3d 3/5] vkd3d-shader/hlsl: Dump only the entry point.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>

Il 16/02/22 06:29, Zebediah Figura ha scritto:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>   libs/vkd3d-shader/hlsl_codegen.c | 19 +------------------
>   1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index eac3513c..20d4ffe9 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -954,23 +954,6 @@ static unsigned int index_instructions(struct hlsl_block *block, unsigned int in
>       return index;
>   }
>
> -static void dump_function_decl(struct rb_entry *entry, void *context)
> -{
> -    struct hlsl_ir_function_decl *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function_decl, entry);
> -    struct hlsl_ctx *ctx = context;
> -
> -    if (func->has_body)
> -        hlsl_dump_function(ctx, func);
> -}
> -
> -static void dump_function(struct rb_entry *entry, void *context)
> -{
> -    struct hlsl_ir_function *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry);
> -    struct hlsl_ctx *ctx = context;
> -
> -    rb_for_each_entry(&func->overloads, dump_function_decl, ctx);
> -}
> -
>   /* Compute the earliest and latest liveness for each variable. In the case that
>    * a variable is accessed inside of a loop, we promote its liveness to extend
>    * to at least the range of the entire loop. Note that we don't need to do this
> @@ -1814,7 +1797,7 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
>       compute_liveness(ctx, entry_func);
>
> if (TRACE_ON())
> -        rb_for_each_entry(&ctx->functions, dump_function, ctx);
> +        hlsl_dump_function(ctx, entry_func);
>
> allocate_temp_registers(ctx, entry_func);
>       if (ctx->profile->major_version < 4)

[PATCH vkd3d 3/5] vkd3d-shader/hlsl: Dump only the entry point.

[Francisco Casas]

Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>

[PATCH vkd3d 3/5] vkd3d-shader/hlsl: Dump only the entry point.

[Matteo Bruni]

On Wed, Feb 16, 2022 at 6:47 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>  libs/vkd3d-shader/hlsl_codegen.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index eac3513c..20d4ffe9 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -954,23 +954,6 @@ static unsigned int index_instructions(struct hlsl_block *block, unsigned int in
>      return index;
>  }
>
> -static void dump_function_decl(struct rb_entry *entry, void *context)
> -{
> -    struct hlsl_ir_function_decl *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function_decl, entry);
> -    struct hlsl_ctx *ctx = context;
> -
> -    if (func->has_body)
> -        hlsl_dump_function(ctx, func);
> -}
> -
> -static void dump_function(struct rb_entry *entry, void *context)
> -{
> -    struct hlsl_ir_function *func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry);
> -    struct hlsl_ctx *ctx = context;
> -
> -    rb_for_each_entry(&func->overloads, dump_function_decl, ctx);
> -}
> -
>  /* Compute the earliest and latest liveness for each variable. In the case that
>   * a variable is accessed inside of a loop, we promote its liveness to extend
>   * to at least the range of the entire loop. Note that we don't need to do this
> @@ -1814,7 +1797,7 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
>      compute_liveness(ctx, entry_func);
>
> if (TRACE_ON())
> -        rb_for_each_entry(&ctx->functions, dump_function, ctx);
> +        hlsl_dump_function(ctx, entry_func);
>
> allocate_temp_registers(ctx, entry_func);
>      if (ctx->profile->major_version < 4)

I think we want to do this only once we have implemented inlining.
Actually, even with inlining, additionally dumping the whole initial
IR doesn't seem like a terrible idea (i.e. instead of removing
dump_function_decl() and dump_function(), moving the call to the start
of hlsl_emit_dxbc()).

[PATCH vkd3d 4/5] vkd3d-shader/hlsl: Return false from hlsl_offset_from_deref() if the offset falls out of bounds.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 libs/vkd3d-shader/hlsl.h                 |  2 +-
 libs/vkd3d-shader/hlsl_codegen.c         | 25 ++++++++++++++++--------
 libs/vkd3d-shader/vkd3d_shader_private.h |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 3a69165e..9b1c9ad3 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -788,7 +788,7 @@ unsigned int hlsl_combine_writemasks(unsigned int first, unsigned int second);
 unsigned int hlsl_map_swizzle(unsigned int swizzle, unsigned int writemask);
 unsigned int hlsl_swizzle_from_writemask(unsigned int writemask);
 
-bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset);
+bool hlsl_offset_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *offset);
 unsigned int hlsl_offset_from_deref_safe(struct hlsl_ctx *ctx, const struct hlsl_deref *deref);
 struct hlsl_reg hlsl_reg_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref,
         const struct hlsl_type *type);
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 20d4ffe9..25f038d4 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -362,15 +362,16 @@ static void copy_propagation_set_value(struct copy_propagation_var_def *var_def,
     }
 }
 
-static struct hlsl_ir_node *copy_propagation_compute_replacement(const struct copy_propagation_state *state,
-        const struct hlsl_deref *deref, unsigned int count, unsigned int *swizzle)
+static struct hlsl_ir_node *copy_propagation_compute_replacement(struct hlsl_ctx *ctx,
+        const struct copy_propagation_state *state, const struct hlsl_deref *deref,
+        unsigned int count, unsigned int *swizzle)
 {
     const struct hlsl_ir_var *var = deref->var;
     struct copy_propagation_var_def *var_def;
     struct hlsl_ir_node *node = NULL;
     unsigned int offset, i;
 
-    if (!hlsl_offset_from_deref(deref, &offset))
+    if (!hlsl_offset_from_deref(ctx, deref, &offset))
         return NULL;
 
     if (!(var_def = copy_propagation_get_var_def(state, var)))
@@ -427,7 +428,7 @@ static bool copy_propagation_transform_load(struct hlsl_ctx *ctx,
             return false;
     }
 
-    if (!(new_node = copy_propagation_compute_replacement(state, &load->src, dimx, &swizzle)))
+    if (!(new_node = copy_propagation_compute_replacement(ctx, state, &load->src, dimx, &swizzle)))
         return false;
 
     if (type->type != HLSL_CLASS_OBJECT)
@@ -448,7 +449,7 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx,
     struct hlsl_ir_node *node;
     unsigned int swizzle;
 
-    if (!(node = copy_propagation_compute_replacement(state, deref, 1, &swizzle)))
+    if (!(node = copy_propagation_compute_replacement(ctx, state, deref, 1, &swizzle)))
         return false;
 
     /* Only HLSL_IR_LOAD can produce an object. */
@@ -481,7 +482,7 @@ static void copy_propagation_record_store(struct hlsl_ctx *ctx, struct hlsl_ir_s
     if (!(var_def = copy_propagation_create_var_def(ctx, state, var)))
         return;
 
-    if (hlsl_offset_from_deref(lhs, &offset))
+    if (hlsl_offset_from_deref(ctx, lhs, &offset))
     {
         unsigned int writemask = store->writemask;
 
@@ -1673,7 +1674,7 @@ static bool type_is_single_reg(const struct hlsl_type *type)
     return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
 }
 
-bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset)
+bool hlsl_offset_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *offset)
 {
     struct hlsl_ir_node *offset_node = deref->offset.node;
 
@@ -1691,6 +1692,14 @@ bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset
         return false;
 
     *offset = hlsl_ir_constant(offset_node)->value[0].u;
+
+    if (*offset >= deref->var->data_type->reg_size)
+    {
+        hlsl_error(ctx, &deref->offset.node->loc, VKD3D_SHADER_ERROR_HLSL_OFFSET_OUT_OF_BOUNDS,
+                "Dereference is out of array bounds.");
+        return false;
+    }
+
     return true;
 }
 
@@ -1698,7 +1707,7 @@ unsigned int hlsl_offset_from_deref_safe(struct hlsl_ctx *ctx, const struct hlsl
 {
     unsigned int offset;
 
-    if (hlsl_offset_from_deref(deref, &offset))
+    if (hlsl_offset_from_deref(ctx, deref, &offset))
         return offset;
 
     hlsl_fixme(ctx, &deref->offset.node->loc, "Dereference with non-constant offset of type %s.",
diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
index 1de67f45..5ab8494c 100644
--- a/libs/vkd3d-shader/vkd3d_shader_private.h
+++ b/libs/vkd3d-shader/vkd3d_shader_private.h
@@ -115,6 +115,7 @@ enum vkd3d_shader_error
     VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION         = 5016,
     VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED             = 5017,
     VKD3D_SHADER_ERROR_HLSL_INVALID_TEXEL_OFFSET        = 5018,
+    VKD3D_SHADER_ERROR_HLSL_OFFSET_OUT_OF_BOUNDS        = 5019,
 
     VKD3D_SHADER_WARNING_HLSL_IMPLICIT_TRUNCATION       = 5300,
 
-- 
2.34.1

[PATCH vkd3d 4/5] vkd3d-shader/hlsl: Return false from hlsl_offset_from_deref() if the offset falls out of bounds.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>
---
Though maybe the error message "Dereference is out of array bounds." is 
not completely appropriate, as it also applies to types that are not 
arrays, right?

Thanks, Giovanni.


Il 16/02/22 06:29, Zebediah Figura ha scritto:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>   libs/vkd3d-shader/hlsl.h                 |  2 +-
>   libs/vkd3d-shader/hlsl_codegen.c         | 25 ++++++++++++++++--------
>   libs/vkd3d-shader/vkd3d_shader_private.h |  1 +
>   3 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index 3a69165e..9b1c9ad3 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -788,7 +788,7 @@ unsigned int hlsl_combine_writemasks(unsigned int first, unsigned int second);
>   unsigned int hlsl_map_swizzle(unsigned int swizzle, unsigned int writemask);
>   unsigned int hlsl_swizzle_from_writemask(unsigned int writemask);
>
> -bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset);
> +bool hlsl_offset_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *offset);
>   unsigned int hlsl_offset_from_deref_safe(struct hlsl_ctx *ctx, const struct hlsl_deref *deref);
>   struct hlsl_reg hlsl_reg_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref,
>           const struct hlsl_type *type);
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 20d4ffe9..25f038d4 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -362,15 +362,16 @@ static void copy_propagation_set_value(struct copy_propagation_var_def *var_def,
>       }
>   }
>
> -static struct hlsl_ir_node *copy_propagation_compute_replacement(const struct copy_propagation_state *state,
> -        const struct hlsl_deref *deref, unsigned int count, unsigned int *swizzle)
> +static struct hlsl_ir_node *copy_propagation_compute_replacement(struct hlsl_ctx *ctx,
> +        const struct copy_propagation_state *state, const struct hlsl_deref *deref,
> +        unsigned int count, unsigned int *swizzle)
>   {
>       const struct hlsl_ir_var *var = deref->var;
>       struct copy_propagation_var_def *var_def;
>       struct hlsl_ir_node *node = NULL;
>       unsigned int offset, i;
>
> -    if (!hlsl_offset_from_deref(deref, &offset))
> +    if (!hlsl_offset_from_deref(ctx, deref, &offset))
>           return NULL;
>
> if (!(var_def = copy_propagation_get_var_def(state, var)))
> @@ -427,7 +428,7 @@ static bool copy_propagation_transform_load(struct hlsl_ctx *ctx,
>               return false;
>       }
>
> -    if (!(new_node = copy_propagation_compute_replacement(state, &load->src, dimx, &swizzle)))
> +    if (!(new_node = copy_propagation_compute_replacement(ctx, state, &load->src, dimx, &swizzle)))
>           return false;
>
> if (type->type != HLSL_CLASS_OBJECT)
> @@ -448,7 +449,7 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx,
>       struct hlsl_ir_node *node;
>       unsigned int swizzle;
>
> -    if (!(node = copy_propagation_compute_replacement(state, deref, 1, &swizzle)))
> +    if (!(node = copy_propagation_compute_replacement(ctx, state, deref, 1, &swizzle)))
>           return false;
>
> /* Only HLSL_IR_LOAD can produce an object. */
> @@ -481,7 +482,7 @@ static void copy_propagation_record_store(struct hlsl_ctx *ctx, struct hlsl_ir_s
>       if (!(var_def = copy_propagation_create_var_def(ctx, state, var)))
>           return;
>
> -    if (hlsl_offset_from_deref(lhs, &offset))
> +    if (hlsl_offset_from_deref(ctx, lhs, &offset))
>       {
>           unsigned int writemask = store->writemask;
>
> @@ -1673,7 +1674,7 @@ static bool type_is_single_reg(const struct hlsl_type *type)
>       return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
>   }
>
> -bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset)
> +bool hlsl_offset_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *offset)
>   {
>       struct hlsl_ir_node *offset_node = deref->offset.node;
>
> @@ -1691,6 +1692,14 @@ bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset
>           return false;
>
> *offset = hlsl_ir_constant(offset_node)->value[0].u;
> +
> +    if (*offset >= deref->var->data_type->reg_size)
> +    {
> +        hlsl_error(ctx, &deref->offset.node->loc, VKD3D_SHADER_ERROR_HLSL_OFFSET_OUT_OF_BOUNDS,
> +                "Dereference is out of array bounds.");
> +        return false;
> +    }
> +
>       return true;
>   }
>
> @@ -1698,7 +1707,7 @@ unsigned int hlsl_offset_from_deref_safe(struct hlsl_ctx *ctx, const struct hlsl
>   {
>       unsigned int offset;
>
> -    if (hlsl_offset_from_deref(deref, &offset))
> +    if (hlsl_offset_from_deref(ctx, deref, &offset))
>           return offset;
>
> hlsl_fixme(ctx, &deref->offset.node->loc, "Dereference with non-constant offset of type %s.",
> diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
> index 1de67f45..5ab8494c 100644
> --- a/libs/vkd3d-shader/vkd3d_shader_private.h
> +++ b/libs/vkd3d-shader/vkd3d_shader_private.h
> @@ -115,6 +115,7 @@ enum vkd3d_shader_error
>       VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION         = 5016,
>       VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED             = 5017,
>       VKD3D_SHADER_ERROR_HLSL_INVALID_TEXEL_OFFSET        = 5018,
> +    VKD3D_SHADER_ERROR_HLSL_OFFSET_OUT_OF_BOUNDS        = 5019,
>
> VKD3D_SHADER_WARNING_HLSL_IMPLICIT_TRUNCATION       = 5300,

[PATCH vkd3d 4/5] vkd3d-shader/hlsl: Return false from hlsl_offset_from_deref() if the offset falls out of bounds.

[Zebediah Figura (she/her)]

On 2/17/22 07:49, Giovanni Mascellani wrote:
> Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>
> ---
> Though maybe the error message "Dereference is out of array bounds." is
> not completely appropriate, as it also applies to types that are not
> arrays, right?

Sure, but in that case it shouldn't be possible to get here.

[PATCH vkd3d 4/5] vkd3d-shader/hlsl: Return false from hlsl_offset_from_deref() if the offset falls out of bounds.

[Giovanni Mascellani]

Hi,

Il 25/02/22 07:00, Zebediah Figura (she/her) ha scritto:
> Sure, but in that case it shouldn't be possible to get here.

I guess there are good chances it will once we implement vector and 
matrix dereference. Of course we might change the message then, but 
since it's easy to forget I'd just leave a more generic message.

Giovanni.

[PATCH vkd3d 4/5] vkd3d-shader/hlsl: Return false from hlsl_offset_from_deref() if the offset falls out of bounds.

[Francisco Casas]

Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>

---

Though I agree with Giovanni.

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Zebediah Figura]

Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
---
 Makefile.am                         |  2 +-
 libs/vkd3d-shader/hlsl.c            |  7 +++++++
 libs/vkd3d-shader/hlsl.h            |  1 +
 libs/vkd3d-shader/hlsl.y            |  2 +-
 libs/vkd3d-shader/hlsl_codegen.c    |  2 +-
 tests/hlsl-struct-array.shader_test | 19 +++++++++++++++++++
 6 files changed, 30 insertions(+), 3 deletions(-)
 create mode 100644 tests/hlsl-struct-array.shader_test

diff --git a/Makefile.am b/Makefile.am
index 75c5e4b1..74e3603c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -93,6 +93,7 @@ vkd3d_shader_tests = \
 	tests/hlsl-state-block-syntax.shader_test \
 	tests/hlsl-static-initializer.shader_test \
 	tests/hlsl-storage-qualifiers.shader_test \
+	tests/hlsl-struct-array.shader_test \
 	tests/hlsl-struct-assignment.shader_test \
 	tests/hlsl-struct-semantics.shader_test \
 	tests/hlsl-vector-indexing.shader_test \
@@ -326,7 +327,6 @@ XFAIL_TESTS = \
 	tests/hlsl-majority-pragma.shader_test \
 	tests/hlsl-majority-typedef.shader_test \
 	tests/hlsl-mul.shader_test \
-	tests/hlsl-nested-arrays.shader_test \
 	tests/hlsl-numeric-constructor-truncation.shader_test \
 	tests/hlsl-numeric-types.shader_test \
 	tests/hlsl-operations.shader_test \
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 8a0f4b01..9c511018 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -202,6 +202,13 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct hlsl_type
     }
 }
 
+/* Returns the size of a type, considered as part of an array of that type.
+ * As such it includes padding after the type. */
+unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type)
+{
+    return align(type->reg_size, 4);
+}
+
 static struct hlsl_type *hlsl_new_type(struct hlsl_ctx *ctx, const char *name, enum hlsl_type_class type_class,
         enum hlsl_base_type base_type, unsigned dimx, unsigned dimy)
 {
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 9b1c9ad3..8ac3e0c5 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -780,6 +780,7 @@ bool hlsl_scope_add_type(struct hlsl_scope *scope, struct hlsl_type *type);
 struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old,
         unsigned int default_majority, unsigned int modifiers);
 unsigned int hlsl_type_component_count(struct hlsl_type *type);
+unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type);
 unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset);
 bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2);
 
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 459ad03b..70109780 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -607,7 +607,7 @@ static struct hlsl_ir_load *add_array_load(struct hlsl_ctx *ctx, struct list *in
         return NULL;
     }
 
-    if (!(c = hlsl_new_uint_constant(ctx, data_type->reg_size, loc)))
+    if (!(c = hlsl_new_uint_constant(ctx, hlsl_type_get_array_element_reg_size(data_type), loc)))
         return NULL;
     list_add_tail(instrs, &c->node.entry);
     if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, index, &c->node)))
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 25f038d4..ccc95c90 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -634,7 +634,7 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr,
     if (type->type != HLSL_CLASS_ARRAY)
         return false;
     element_type = type->e.array.type;
-    element_size = element_type->reg_size;
+    element_size = hlsl_type_get_array_element_reg_size(element_type);
 
     for (i = 0; i < type->e.array.elements_count; ++i)
     {
diff --git a/tests/hlsl-struct-array.shader_test b/tests/hlsl-struct-array.shader_test
new file mode 100644
index 00000000..01996a50
--- /dev/null
+++ b/tests/hlsl-struct-array.shader_test
@@ -0,0 +1,19 @@
+% In SM4, arrays are always aligned to the next vec4, although structs are not.
+
+[pixel shader]
+uniform struct
+{
+    float p, q;
+} colours[3];
+
+float4 main() : sv_target
+{
+    return float4(colours[0].q, colours[1].p, colours[2].q, colours[2].p);
+}
+
+[test]
+uniform  0 float4 0.1 0.2 0.0 0.0
+uniform  4 float4 0.3 0.4 0.0 0.0
+uniform  8 float4 0.5 0.6 0.0 0.0
+draw quad
+probe all rgba (0.2, 0.3, 0.6, 0.5)
-- 
2.34.1

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Giovanni Mascellani]

Hi,

I think the patch is mostly ok, but both hlsl-struct-array.shader_test 
and hlsl-nested-arrays.shader_test are failing here, with basically the 
same error:
> trace:vkd3d_shader_compile: <anonymous>:7:28: E5017: Aborting due to not yet implemented feature: SM4 uint "+" expression.
> trace:vkd3d_shader_compile: 
> trace:vkd3d_shader_compile: <anonymous>:7:28: E5017: Aborting due to not yet implemented feature: Dereference with non-constant offset of type HLSL_IR_EXPR.
> trace:vkd3d_shader_compile: <anonymous>:7:48: E5017: Aborting due to not yet implemented feature: SM4 uint "+" expression.
> trace:vkd3d_shader_compile: 
> trace:vkd3d_shader_compile: <anonymous>:7:48: E5017: Aborting due to not yet implemented feature: Dereference with non-constant offset of type HLSL_IR_EXPR.
> trace:vkd3d_shader_compile: <anonymous>:7:68: E5017: Aborting due to not yet implemented feature: SM4 uint "+" expression.
> trace:vkd3d_shader_compile: 
> trace:vkd3d_shader_compile: <anonymous>:7:68: E5017: Aborting due to not yet implemented feature: Dereference with non-constant offset of type HLSL_IR_EXPR.
> trace:vkd3d_shader_compile: <anonymous>:7:88: E5017: Aborting due to not yet implemented feature: SM4 uint "+" expression.
> trace:vkd3d_shader_compile: 
> trace:vkd3d_shader_compile: <anonymous>:7:88: E5017: Aborting due to not yet implemented feature: Dereference with non-constant offset of type HLSL_IR_EXPR.
> shader_runner:69:Section [test], line 14: Test failed: Failed to compile shader, hr 0x80004001.
> shader_runner:246:Section [test], line 14: Test failed: Got {0.00000000e+00, 0.00000000e+00, 0.00000000e+00, 0.00000000e+00}, expected {2.00000003e-01, 3.00000012e-01, 6.00000024e-01, 5.00000000e-01} at (0, 0).

Could it be that you tested them on a different branch?

Thanks, Giovanni.


Il 16/02/22 06:29, Zebediah Figura ha scritto:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
>   Makefile.am                         |  2 +-
>   libs/vkd3d-shader/hlsl.c            |  7 +++++++
>   libs/vkd3d-shader/hlsl.h            |  1 +
>   libs/vkd3d-shader/hlsl.y            |  2 +-
>   libs/vkd3d-shader/hlsl_codegen.c    |  2 +-
>   tests/hlsl-struct-array.shader_test | 19 +++++++++++++++++++
>   6 files changed, 30 insertions(+), 3 deletions(-)
>   create mode 100644 tests/hlsl-struct-array.shader_test
>
> diff --git a/Makefile.am b/Makefile.am
> index 75c5e4b1..74e3603c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -93,6 +93,7 @@ vkd3d_shader_tests = \
>   tests/hlsl-state-block-syntax.shader_test \
>   tests/hlsl-static-initializer.shader_test \
>   tests/hlsl-storage-qualifiers.shader_test \
> +	tests/hlsl-struct-array.shader_test \
>   tests/hlsl-struct-assignment.shader_test \
>   tests/hlsl-struct-semantics.shader_test \
>   tests/hlsl-vector-indexing.shader_test \
> @@ -326,7 +327,6 @@ XFAIL_TESTS = \
>   tests/hlsl-majority-pragma.shader_test \
>   tests/hlsl-majority-typedef.shader_test \
>   tests/hlsl-mul.shader_test \
> -	tests/hlsl-nested-arrays.shader_test \
>   tests/hlsl-numeric-constructor-truncation.shader_test \
>   tests/hlsl-numeric-types.shader_test \
>   tests/hlsl-operations.shader_test \
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index 8a0f4b01..9c511018 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c
> @@ -202,6 +202,13 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct hlsl_type
>       }
>   }
>
> +/* Returns the size of a type, considered as part of an array of that type.
> + * As such it includes padding after the type. */
> +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type)
> +{
> +    return align(type->reg_size, 4);
> +}
> +
>   static struct hlsl_type *hlsl_new_type(struct hlsl_ctx *ctx, const char *name, enum hlsl_type_class type_class,
>           enum hlsl_base_type base_type, unsigned dimx, unsigned dimy)
>   {
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index 9b1c9ad3..8ac3e0c5 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -780,6 +780,7 @@ bool hlsl_scope_add_type(struct hlsl_scope *scope, struct hlsl_type *type);
>   struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old,
>           unsigned int default_majority, unsigned int modifiers);
>   unsigned int hlsl_type_component_count(struct hlsl_type *type);
> +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type);
>   unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset);
>   bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2);
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 459ad03b..70109780 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -607,7 +607,7 @@ static struct hlsl_ir_load *add_array_load(struct hlsl_ctx *ctx, struct list *in
>           return NULL;
>       }
>
> -    if (!(c = hlsl_new_uint_constant(ctx, data_type->reg_size, loc)))
> +    if (!(c = hlsl_new_uint_constant(ctx, hlsl_type_get_array_element_reg_size(data_type), loc)))
>           return NULL;
>       list_add_tail(instrs, &c->node.entry);
>       if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, index, &c->node)))
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 25f038d4..ccc95c90 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -634,7 +634,7 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr,
>       if (type->type != HLSL_CLASS_ARRAY)
>           return false;
>       element_type = type->e.array.type;
> -    element_size = element_type->reg_size;
> +    element_size = hlsl_type_get_array_element_reg_size(element_type);
>
> for (i = 0; i < type->e.array.elements_count; ++i)
>       {
> diff --git a/tests/hlsl-struct-array.shader_test b/tests/hlsl-struct-array.shader_test
> new file mode 100644
> index 00000000..01996a50
> --- /dev/null
> +++ b/tests/hlsl-struct-array.shader_test
> @@ -0,0 +1,19 @@
> +% In SM4, arrays are always aligned to the next vec4, although structs are not.
> +
> +[pixel shader]
> +uniform struct
> +{
> +    float p, q;
> +} colours[3];
> +
> +float4 main() : sv_target
> +{
> +    return float4(colours[0].q, colours[1].p, colours[2].q, colours[2].p);
> +}
> +
> +[test]
> +uniform  0 float4 0.1 0.2 0.0 0.0
> +uniform  4 float4 0.3 0.4 0.0 0.0
> +uniform  8 float4 0.5 0.6 0.0 0.0
> +draw quad
> +probe all rgba (0.2, 0.3, 0.6, 0.5)

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Francisco Casas]

Hi,

The failing tests aside, this padding rule for array elements is 
an interesting find.

If this is correct, wouldn't that mean that we also have to use
hlsl_type_get_array_element_reg_size() in hlsl_type_calculate_reg_size()?

In particular, here
---
        case HLSL_CLASS_ARRAY:
        {
            unsigned int element_size = type->e.array.type->reg_size;

            assert(element_size);
            if (is_sm4)
                type->reg_size = (type->e.array.elements_count - 1) * align(element_size, 4) + element_size;
            else
                type->reg_size = type->e.array.elements_count * element_size;
            break;
        }
---

Also, it makes me wonder whether this rule only applies in sm4 and/or to uniform
arrays (I don't know a quick way to test that).

If I am not mistaken and we indeed have to change hlsl_type_calculate_reg_size(), 
then we would have to also take into account the padding in the
HLSL_CLASS_STRUCT case, when the struct contains array fields.


Thanks,
Francisco.


February 16, 2022 2:47 AM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
> Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> ---
> Makefile.am | 2 +-
> libs/vkd3d-shader/hlsl.c | 7 +++++++
> libs/vkd3d-shader/hlsl.h | 1 +
> libs/vkd3d-shader/hlsl.y | 2 +-
> libs/vkd3d-shader/hlsl_codegen.c | 2 +-
> tests/hlsl-struct-array.shader_test | 19 +++++++++++++++++++
> 6 files changed, 30 insertions(+), 3 deletions(-)
> create mode 100644 tests/hlsl-struct-array.shader_test
>
> diff --git a/Makefile.am b/Makefile.am
> index 75c5e4b1..74e3603c 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -93,6 +93,7 @@ vkd3d_shader_tests = \
> tests/hlsl-state-block-syntax.shader_test \
> tests/hlsl-static-initializer.shader_test \
> tests/hlsl-storage-qualifiers.shader_test \
> + tests/hlsl-struct-array.shader_test \
> tests/hlsl-struct-assignment.shader_test \
> tests/hlsl-struct-semantics.shader_test \
> tests/hlsl-vector-indexing.shader_test \
> @@ -326,7 +327,6 @@ XFAIL_TESTS = \
> tests/hlsl-majority-pragma.shader_test \
> tests/hlsl-majority-typedef.shader_test \
> tests/hlsl-mul.shader_test \
> - tests/hlsl-nested-arrays.shader_test \
> tests/hlsl-numeric-constructor-truncation.shader_test \
> tests/hlsl-numeric-types.shader_test \
> tests/hlsl-operations.shader_test \
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index 8a0f4b01..9c511018 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c
> @@ -202,6 +202,13 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct
> hlsl_type
> }
> }
>
> +/* Returns the size of a type, considered as part of an array of that type.
> + * As such it includes padding after the type. */
> +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type)
> +{
> + return align(type->reg_size, 4);
> +}
> +
> static struct hlsl_type *hlsl_new_type(struct hlsl_ctx *ctx, const char *name, enum hlsl_type_class
> type_class,
> enum hlsl_base_type base_type, unsigned dimx, unsigned dimy)
> {
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index 9b1c9ad3..8ac3e0c5 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -780,6 +780,7 @@ bool hlsl_scope_add_type(struct hlsl_scope *scope, struct hlsl_type *type);
> struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old,
> unsigned int default_majority, unsigned int modifiers);
> unsigned int hlsl_type_component_count(struct hlsl_type *type);
> +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type);
> unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset);
> bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2);
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 459ad03b..70109780 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -607,7 +607,7 @@ static struct hlsl_ir_load *add_array_load(struct hlsl_ctx *ctx, struct list
> *in
> return NULL;
> }
>
> - if (!(c = hlsl_new_uint_constant(ctx, data_type->reg_size, loc)))
> + if (!(c = hlsl_new_uint_constant(ctx, hlsl_type_get_array_element_reg_size(data_type), loc)))
> return NULL;
> list_add_tail(instrs, &c->node.entry);
> if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, index, &c->node)))
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 25f038d4..ccc95c90 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -634,7 +634,7 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node
> *instr,
> if (type->type != HLSL_CLASS_ARRAY)
> return false;
> element_type = type->e.array.type;
> - element_size = element_type->reg_size;
> + element_size = hlsl_type_get_array_element_reg_size(element_type);
>
> for (i = 0; i < type->e.array.elements_count; ++i)
> {
> diff --git a/tests/hlsl-struct-array.shader_test b/tests/hlsl-struct-array.shader_test
> new file mode 100644
> index 00000000..01996a50
> --- /dev/null
> +++ b/tests/hlsl-struct-array.shader_test
> @@ -0,0 +1,19 @@
> +% In SM4, arrays are always aligned to the next vec4, although structs are not.
> +
> +[pixel shader]
> +uniform struct
> +{
> + float p, q;
> +} colours[3];
> +
> +float4 main() : sv_target
> +{
> + return float4(colours[0].q, colours[1].p, colours[2].q, colours[2].p);
> +}
> +
> +[test]
> +uniform 0 float4 0.1 0.2 0.0 0.0
> +uniform 4 float4 0.3 0.4 0.0 0.0
> +uniform 8 float4 0.5 0.6 0.0 0.0
> +draw quad
> +probe all rgba (0.2, 0.3, 0.6, 0.5)
> -- 
> 2.34.1

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Matteo Bruni]

On Fri, Feb 18, 2022 at 12:05 AM Francisco Casas <fcasas(a)codeweavers.com> wrote:
> Hi,
>
> The failing tests aside, this padding rule for array elements is
> an interesting find.
>
> If this is correct, wouldn't that mean that we also have to use
> hlsl_type_get_array_element_reg_size() in hlsl_type_calculate_reg_size()?
>
> In particular, here
> ---
>         case HLSL_CLASS_ARRAY:
>         {
>             unsigned int element_size = type->e.array.type->reg_size;
>
> assert(element_size);
>             if (is_sm4)
>                 type->reg_size = (type->e.array.elements_count - 1) * align(element_size, 4) + element_size;
>             else
>                 type->reg_size = type->e.array.elements_count * element_size;
>             break;
>         }
> ---
>
> Also, it makes me wonder whether this rule only applies in sm4 and/or to uniform
> arrays (I don't know a quick way to test that).

Interestingly, this is the code generated for
hlsl-struct-array.shader_test by native for ps_2_0:

    ps_2_0
    mov r0.x, c1.x
    mov r0.y, c2.x
    mov r0.z, c5.x
    mov r0.w, c4.x
    mov oC0, r0

i.e. a full 4-component register is allocated for each struct element.
I seem to recall having seen this before, we might even have tests for
it?

Anyway, you make all valid points in this reply. I guess we could add
more tests and then fix stuff accordingly in follow up patches, or get
a better picture first and then implement it correctly right away. I'm
resending the patch with minimal changes, since I have it here anyway,
but (all of you) feel free to sign-off or not.
> If I am not mistaken and we indeed have to change hlsl_type_calculate_reg_size(),
> then we would have to also take into account the padding in the
> HLSL_CLASS_STRUCT case, when the struct contains array fields.
>
> Thanks,
> Francisco.
>
> February 16, 2022 2:47 AM, "Zebediah Figura" <zfigura(a)codeweavers.com> wrote:
> > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
> > ---
> > Makefile.am | 2 +-
> > libs/vkd3d-shader/hlsl.c | 7 +++++++
> > libs/vkd3d-shader/hlsl.h | 1 +
> > libs/vkd3d-shader/hlsl.y | 2 +-
> > libs/vkd3d-shader/hlsl_codegen.c | 2 +-
> > tests/hlsl-struct-array.shader_test | 19 +++++++++++++++++++
> > 6 files changed, 30 insertions(+), 3 deletions(-)
> > create mode 100644 tests/hlsl-struct-array.shader_test
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 75c5e4b1..74e3603c 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -93,6 +93,7 @@ vkd3d_shader_tests = \
> > tests/hlsl-state-block-syntax.shader_test \
> > tests/hlsl-static-initializer.shader_test \
> > tests/hlsl-storage-qualifiers.shader_test \
> > + tests/hlsl-struct-array.shader_test \
> > tests/hlsl-struct-assignment.shader_test \
> > tests/hlsl-struct-semantics.shader_test \
> > tests/hlsl-vector-indexing.shader_test \
> > @@ -326,7 +327,6 @@ XFAIL_TESTS = \
> > tests/hlsl-majority-pragma.shader_test \
> > tests/hlsl-majority-typedef.shader_test \
> > tests/hlsl-mul.shader_test \
> > - tests/hlsl-nested-arrays.shader_test \
> > tests/hlsl-numeric-constructor-truncation.shader_test \
> > tests/hlsl-numeric-types.shader_test \
> > tests/hlsl-operations.shader_test \
> > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> > index 8a0f4b01..9c511018 100644
> > --- a/libs/vkd3d-shader/hlsl.c
> > +++ b/libs/vkd3d-shader/hlsl.c
> > @@ -202,6 +202,13 @@ static void hlsl_type_calculate_reg_size(struct hlsl_ctx *ctx, struct
> > hlsl_type
> > }
> > }
> >
> > +/* Returns the size of a type, considered as part of an array of that type.
> > + * As such it includes padding after the type. */
> > +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type)
> > +{
> > + return align(type->reg_size, 4);
> > +}
> > +
> > static struct hlsl_type *hlsl_new_type(struct hlsl_ctx *ctx, const char *name, enum hlsl_type_class
> > type_class,
> > enum hlsl_base_type base_type, unsigned dimx, unsigned dimy)
> > {
> > diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> > index 9b1c9ad3..8ac3e0c5 100644
> > --- a/libs/vkd3d-shader/hlsl.h
> > +++ b/libs/vkd3d-shader/hlsl.h
> > @@ -780,6 +780,7 @@ bool hlsl_scope_add_type(struct hlsl_scope *scope, struct hlsl_type *type);
> > struct hlsl_type *hlsl_type_clone(struct hlsl_ctx *ctx, struct hlsl_type *old,
> > unsigned int default_majority, unsigned int modifiers);
> > unsigned int hlsl_type_component_count(struct hlsl_type *type);
> > +unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type);
> > unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset);
> > bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2);
> >
> > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> > index 459ad03b..70109780 100644
> > --- a/libs/vkd3d-shader/hlsl.y
> > +++ b/libs/vkd3d-shader/hlsl.y
> > @@ -607,7 +607,7 @@ static struct hlsl_ir_load *add_array_load(struct hlsl_ctx *ctx, struct list
> > *in
> > return NULL;
> > }
> >
> > - if (!(c = hlsl_new_uint_constant(ctx, data_type->reg_size, loc)))
> > + if (!(c = hlsl_new_uint_constant(ctx, hlsl_type_get_array_element_reg_size(data_type), loc)))
> > return NULL;
> > list_add_tail(instrs, &c->node.entry);
> > if (!(mul = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, index, &c->node)))
> > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> > index 25f038d4..ccc95c90 100644
> > --- a/libs/vkd3d-shader/hlsl_codegen.c
> > +++ b/libs/vkd3d-shader/hlsl_codegen.c
> > @@ -634,7 +634,7 @@ static bool split_array_copies(struct hlsl_ctx *ctx, struct hlsl_ir_node
> > *instr,
> > if (type->type != HLSL_CLASS_ARRAY)
> > return false;
> > element_type = type->e.array.type;
> > - element_size = element_type->reg_size;
> > + element_size = hlsl_type_get_array_element_reg_size(element_type);
> >
> > for (i = 0; i < type->e.array.elements_count; ++i)
> > {
> > diff --git a/tests/hlsl-struct-array.shader_test b/tests/hlsl-struct-array.shader_test
> > new file mode 100644
> > index 00000000..01996a50
> > --- /dev/null
> > +++ b/tests/hlsl-struct-array.shader_test
> > @@ -0,0 +1,19 @@
> > +% In SM4, arrays are always aligned to the next vec4, although structs are not.
> > +
> > +[pixel shader]
> > +uniform struct
> > +{
> > + float p, q;
> > +} colours[3];
> > +
> > +float4 main() : sv_target
> > +{
> > + return float4(colours[0].q, colours[1].p, colours[2].q, colours[2].p);
> > +}
> > +
> > +[test]
> > +uniform 0 float4 0.1 0.2 0.0 0.0
> > +uniform 4 float4 0.3 0.4 0.0 0.0
> > +uniform 8 float4 0.5 0.6 0.0 0.0
> > +draw quad
> > +probe all rgba (0.2, 0.3, 0.6, 0.5)
> > --
> > 2.34.1

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Zebediah Figura (she/her)]

On 2/17/22 17:04, Francisco Casas wrote:
> Hi,
>
> The failing tests aside, this padding rule for array elements is 
> an interesting find.
>
> If this is correct, wouldn't that mean that we also have to use
> hlsl_type_get_array_element_reg_size() in hlsl_type_calculate_reg_size()?
>
> In particular, here
> ---
>         case HLSL_CLASS_ARRAY:
>         {
>             unsigned int element_size = type->e.array.type->reg_size;
>
> assert(element_size);
>             if (is_sm4)
>                 type->reg_size = (type->e.array.elements_count - 1) * align(element_size, 4) + element_size;
>             else
>                 type->reg_size = type->e.array.elements_count * element_size;
>             break;
>         }
> ---
>
> Also, it makes me wonder whether this rule only applies in sm4 and/or to uniform
> arrays (I don't know a quick way to test that).
>
> If I am not mistaken and we indeed have to change hlsl_type_calculate_reg_size(), 
> then we would have to also take into account the padding in the
> HLSL_CLASS_STRUCT case, when the struct contains array fields.

Well, sort of. The "align(element_size, 4)" there is basically an
open-coded hlsl_type_get_array_element_reg_size(), and could I guess get
replaced with that.

Note that sm4 structs, unlike C structs, never have padding on the end.
We should have tests for that in d3dcompiler.

Note also that in sm1, *all* uniforms are aligned to the nearest whole
register, and we set reg_size accordingly. So the answer to "does this
apply to sm1" is basically "it's a moot point".

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Francisco Casas]

February 25, 2022 3:11 AM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
> Well, sort of. The "align(element_size, 4)" there is basically an
> open-coded hlsl_type_get_array_element_reg_size(), and could I guess get
> replaced with that.
>
> Note that sm4 structs, unlike C structs, never have padding on the end.
> We should have tests for that in d3dcompiler.
>
> Note also that in sm1, *all* uniforms are aligned to the nearest whole
> register, and we set reg_size accordingly. So the answer to "does this
> apply to sm1" is basically "it's a moot point".

Thank you! I understand now.

I should also update the array initializers patch in the same way.


Best regards,
Francisco.

[PATCH vkd3d 5/5] vkd3d-shader/hlsl: Correctly calculate offsets for array elements.

[Giovanni Mascellani]

Hi,

Il 16/02/22 06:29, Zebediah Figura ha scritto:
> +% In SM4, arrays are always aligned to the next vec4, although structs are not.

Another minor comment: the point here is not the alignment of "arrays", 
but of "array elements". Which kind of implies the alignment for the 
whole array, but it's a stricter condition.

Giovanni.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com>
---
Not a big issue, but...

Il 16/02/22 06:29, Zebediah Figura ha scritto:
> +% It's legal to call an undefined function in unused code, though.
> +
> +[pixel shader]

Here you write the commend before the test section header.
> +[pixel shader fail]
> +
> +% Returning a void expression from a void function is legal in C, but not in HLSL.

And here the other way around.

Giovanni.

[Francisco Casas]

Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>