[PATCH 0/1] MR54: vkd3d-shader/hlsl: Initialize static variables to 0 by default.
We are currently not initializing static values to zero by default. Consider the following shader: ```hlsl static float4 va; float4 main() : sv_target { return va; } ``` we get the following output: ``` ps_5_0 dcl_output o0.xyzw dcl_temps 2 mov r0.xyzw, r1.xyzw mov o0.xyzw, r0.xyzw ret ``` where r1.xyzw is not initialized. This patch solves this by assigning the static variable the value of an uint 0, and thus, relying on complex broadcasts. This seems to be the behaviour of the the native compiler, since it retrieves the following error on a shader that lacks an initializer on a data type with object components: ``` error X3017: cannot convert from 'uint' to 'struct <unnamed>' ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
From: Francisco Casas <fcasas(a)codeweavers.com> We are currently not initializing static values to zero by default. Consider the following shader: ```hlsl static float4 va; float4 main() : sv_target { return va; } ``` we get the following output: ``` ps_5_0 dcl_output o0.xyzw dcl_temps 2 mov r0.xyzw, r1.xyzw mov o0.xyzw, r0.xyzw ret ``` where r1.xyzw is not initialized. This patch solves this by assigning the static variable the value of an uint 0, and thus, relying on complex broadcasts. This seems to be the behaviour of the the native compiler, since it retrieves the following error on a shader that lacks an initializer on a data type with object components: ``` error X3017: cannot convert from 'uint' to 'struct <unnamed>' ``` --- libs/vkd3d-shader/hlsl.y | 35 ++++++++++++++++++++++++++++++++++ tests/hlsl-invalid.shader_test | 8 ++++++++ 2 files changed, 43 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index eedc85bd..a10f7308 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2167,6 +2167,41 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v->initializer.args); vkd3d_free(v->initializer.instrs); } + else if (var->modifiers & HLSL_STORAGE_STATIC) + { + struct hlsl_ir_constant *zero; + struct hlsl_ir_load *load; + + /* Initialize statics to zero by default. */ + + if (type_has_object_components(var->data_type, false)) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Static variable cannot be initialized to 0 if it has object components."); + vkd3d_free(v); + continue; + } + + if (!(zero = hlsl_new_uint_constant(ctx, 0, &var->loc))) + { + vkd3d_free(v); + continue; + } + list_add_tail(&ctx->static_initializers, &zero->node.entry); + + if (!(load = hlsl_new_var_load(ctx, var, var->loc))) + { + vkd3d_free(v); + continue; + } + list_add_tail(&ctx->static_initializers, &load->node.entry); + + if (!add_assignment(ctx, &ctx->static_initializers, &load->node, ASSIGN_OP_ASSIGN, &zero->node)) + { + vkd3d_free(v); + continue; + } + } vkd3d_free(v); } vkd3d_free(var_list); diff --git a/tests/hlsl-invalid.shader_test b/tests/hlsl-invalid.shader_test index 1b39d7f0..3e18961b 100644 --- a/tests/hlsl-invalid.shader_test +++ b/tests/hlsl-invalid.shader_test @@ -264,3 +264,11 @@ float4 main() : sv_target { return 1.0; } + +[pixel shader fail] +static uint i; +float4 main() : sv_target { return 1/i; } + +[pixel shader fail] +static Texture2D tex; +float4 main() : sv_target { return 0; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
{ return 1.0; } + +[pixel shader fail] +static uint i; +float4 main() : sv_target { return 1/i; } + +[pixel shader fail] +static Texture2D tex; +float4 main() : sv_target { return 0; } This seems to compile here, at least with profiles `ps_4_0`, `ps_5_0` and `ps_5_1`: https://shader-playground.timjones.io/4e83a24178541aea41b6cef402cd564f.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18631
Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
{ return 1.0; } + +[pixel shader fail] +static uint i; +float4 main() : sv_target { return 1/i; } + +[pixel shader fail] +static Texture2D tex; +float4 main() : sv_target { return 0; } I think they should be formatted properly, even if they are really short.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18632
Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
{ return 1.0; } + +[pixel shader fail] +static uint i; +float4 main() : sv_target { return 1/i; } It's good to test for this failure, but we should probably also have a succeeding test for the usual case (having a static numeric variable, returning it and showing it is zero).
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18633
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
vkd3d_free(v->initializer.args); vkd3d_free(v->initializer.instrs); } + else if (var->modifiers & HLSL_STORAGE_STATIC) + { + struct hlsl_ir_constant *zero; + struct hlsl_ir_load *load; + + /* Initialize statics to zero by default. */ + + if (type_has_object_components(var->data_type, false)) + { + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER, + "Static variable cannot be initialized to 0 if it has object components.");
Actually, it seems that a static variable with objects is fine if it contains *only* objects. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18634
The MR is mostly good, and given that it's a bugfix it is also worthy of a freeze exception, but there a few issues to address. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18635
On Mon Dec 5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
This seems to compile here, at least with profiles `ps_4_0`, `ps_5_0` and `ps_5_1`: https://shader-playground.timjones.io/4e83a24178541aea41b6cef402cd564f. hmm, so it doesn't compile with 9.29.952.3111 (which I was using for testing) but it does with 10.0.10011.16384.
I guess we will have to add a way of having uninitialized objects, because in 10.0.10011.16384, the following shader: ```hlsl static Texture2D tex; float4 main() : sv_target { return tex.Load(int3(0, 0, 0)); } ``` results in: ``` error X4000: variable 'tex' used without having been completely initialized ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18648
On Mon Dec 5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
I think they should be formatted properly, even if they are really short. Okay, there are some other tests with this shorter format in this file (and `hlsl-attributes.shader_test`) though.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18649
On Mon Dec 5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
It's good to test for this failure, but we should probably also have a succeeding test for the usual case (having a static numeric variable, returning it and showing it is zero). I wanted to avoid a nondeterministic test. If the test depends on an uninitialized value, it may succeed or fail randomly.
This one has the nicety that is constant folding that triggers the error iff the value is actually initialized to zero, otherwise, it has no way of knowing that the denominator will be zero. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18650
On Mon Dec 5 13:04:06 2022 +0000, Giovanni Mascellani wrote:
Actually, it seems that a static variable with objects is fine if it contains *only* objects. Yep, seems to be the case in 10.0.10011.16384, but is not in 9.29.952.3111.
Should we go for the most permissive approach? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18651
On Mon Dec 5 15:28:01 2022 +0000, Francisco Casas wrote:
Okay, there are some other tests with this shorter format in this file (and `hlsl-attributes.shader_test`) though. Yeah, I don't think it's a big deal either way, personally, at least not in the tests, and not for very simple tests.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18676
On Mon Dec 5 15:24:56 2022 +0000, Francisco Casas wrote:
hmm, so it doesn't compile with 9.29.952.3111 (which I was using for testing) but it does with 10.0.10011.16384. I guess we will have to add a way of having uninitialized objects, because in 10.0.10011.16384, the following shader: ```hlsl static Texture2D tex; float4 main() : sv_target { return tex.Load(int3(0, 0, 0)); } ``` results in: ``` error X4000: variable 'tex' used without having been completely initialized ``` Is that a version number on d3dcompiler_47?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18677
On Mon Dec 5 15:34:44 2022 +0000, Francisco Casas wrote:
Yep, seems to be the case in 10.0.10011.16384, but is not in 9.29.952.3111. Should we go for the most permissive approach? I would advocate for taking the most up-to-date behaviour (especially because it's easier, and more restrictive). We may at some point need to add a compiler switch to turn off that check, but it may make the most sense to just wait until a program actually needs it.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18678
On Mon Dec 5 19:12:45 2022 +0000, Zebediah Figura wrote:
I would advocate for taking the most up-to-date behaviour (especially because it's easier, and more restrictive). We may at some point need to add a compiler switch to turn off that check, but it may make the most sense to just wait until a program actually needs it. The most up-to-date behavior (10.0.10011.16384) is also the most permissive, since it allows for uninitialized static variables with objects (to which a value can be initialized later) instead of just simply not allowing them.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18693
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)