Signed-off-by: Zebediah Figura zfigura@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)
Signed-off-by: Zebediah Figura zfigura@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; +}
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com
Il 16/02/22 06:29, Zebediah Figura ha scritto:
Signed-off-by: Zebediah Figura zfigura@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;
+}
Hello,
February 16, 2022 2:47 AM, "Zebediah Figura" zfigura@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
On Thu, Feb 17, 2022 at 5:17 PM Francisco Casas fcasas@codeweavers.com wrote:
Hello,
February 16, 2022 2:47 AM, "Zebediah Figura" zfigura@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.
Signed-off-by: Zebediah Figura zfigura@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)
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com
Il 16/02/22 06:29, Zebediah Figura ha scritto:
Signed-off-by: Zebediah Figura zfigura@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)
Signed-off-by: Francisco Casas fcasas@codeweavers.com
On Wed, Feb 16, 2022 at 6:47 AM Zebediah Figura zfigura@codeweavers.com wrote:
Signed-off-by: Zebediah Figura zfigura@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()).
Signed-off-by: Zebediah Figura zfigura@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,
Signed-off-by: Giovanni Mascellani gmascellani@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@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)
- 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,
On 2/17/22 07:49, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani gmascellani@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.
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.
Signed-off-by: Zebediah Figura zfigura@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)
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@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)
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@codeweavers.com wrote:
Signed-off-by: Zebediah Figura zfigura@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
On Fri, Feb 18, 2022 at 12:05 AM Francisco Casas fcasas@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@codeweavers.com wrote:
Signed-off-by: Zebediah Figura zfigura@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
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".
February 25, 2022 3:11 AM, "Zebediah Figura (she/her)" zfigura@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.
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.
Signed-off-by: Giovanni Mascellani gmascellani@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 -
Giovanni Mascellani -
Matteo Bruni -
Zebediah Figura -
Zebediah Figura (she/her)