[PATCH v2 0/2] 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>' ``` -- v2: vkd3d-shader/hlsl: Allow uninitialized static objects, as long as they are not referenced. vkd3d-shader/hlsl: Initialize static variables to 0 by default. 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 9.29.952.3111 version of 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 | 34 +++++++++++++++++++ tests/hlsl-static-initializer.shader_test | 41 +++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index eedc85bd..91bc3192 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2167,6 +2167,40 @@ 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_fixme(ctx, &var->loc, "Uninitialized static objects."); + 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-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 0f53f4d1..65ae23eb 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -14,3 +14,44 @@ float4 main() : sv_target [test] todo draw quad probe all rgba (0.8, 0.0, 0.0, 0.0) + + +[pixel shader fail] +static uint i; + +float4 main() : sv_target +{ + return 1 / i; +} + + +[pixel shader fail todo] +static Texture2D tex; + +float4 main() : sv_target +{ + return tex.Load(int3(0, 0, 0)); +} + + +[pixel shader todo] +// This is allowed in 10.0.10011.16384 but not in 9.29.952.3111 +static Texture2D tex; + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader todo] +// This is allowed in 10.0.10011.16384 but not in 9.29.952.3111 +static Texture2D tex; + +float4 main() : sv_target +{ + if (0) + return tex.Load(int3(0, 0, 0)); + else + return float4(0, 1, 2, 3); +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
From: Francisco Casas <fcasas(a)codeweavers.com> Note that in the future we should call validate_static_object_references() after dce, and dce has to be able to delete resource stores and resource loads, because shaders such as this compile (at least in 10.0.10011.16384): ``` static Texture2D tex[2]; uniform int i; float4 main() : sv_target { if (0) return tex[i].Load(int3(0, 0, 0)); else return float4(0, 1, 2, 3); } ``` The same for some of the "todo" tests in hlsl-static-initializer.shader_test. Doing this requires to move `transform_deref_paths_into_offsets` after `dce`, which implies translating `compute_liveness` and `dce` to index paths. An alternative course of action is to write the checks in hlsl_sm1.c and hlsl_sm4_write.c instead. --- libs/vkd3d-shader/hlsl.y | 1 - libs/vkd3d-shader/hlsl_codegen.c | 30 ++++++++++++++++++++--- tests/hlsl-static-initializer.shader_test | 4 +-- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 91bc3192..0b6eb105 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2176,7 +2176,6 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if (type_has_object_components(var->data_type, false)) { - hlsl_fixme(ctx, &var->loc, "Uninitialized static objects."); vkd3d_free(v); continue; } diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6e4168fc..33fc8754 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1016,12 +1016,28 @@ static bool validate_static_object_references(struct hlsl_ctx *ctx, struct hlsl_ load->resource.var->name); note_non_static_deref_expressions(ctx, &load->resource, "loaded resource"); } - if (load->sampler.var && !hlsl_component_index_range_from_deref(ctx, &load->sampler, &start, &count)) + else if (load->resource.var->modifiers & HLSL_STORAGE_STATIC) { hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, - "Resource load sampler from \"%s\" must be determinable at compile time.", - load->sampler.var->name); - note_non_static_deref_expressions(ctx, &load->sampler, "resource load sampler"); + "Loaded resource from static variable \"%s\" must be initialized.", + load->resource.var->name); + } + + if (load->sampler.var) + { + if (!hlsl_component_index_range_from_deref(ctx, &load->sampler, &start, &count)) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Resource load sampler from \"%s\" must be determinable at compile time.", + load->sampler.var->name); + note_non_static_deref_expressions(ctx, &load->sampler, "resource load sampler"); + } + else if (load->sampler.var->modifiers & HLSL_STORAGE_STATIC) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Loaded sampler from static variable \"%s\" must be initialized.", + load->sampler.var->name); + } } } else if (instr->type == HLSL_IR_RESOURCE_STORE) @@ -1035,6 +1051,12 @@ static bool validate_static_object_references(struct hlsl_ctx *ctx, struct hlsl_ store->resource.var->name); note_non_static_deref_expressions(ctx, &store->resource, "accessed resource"); } + else if (store->resource.var->modifiers & HLSL_STORAGE_STATIC) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Accesed resource from static variable \"%s\" must be initialized.", + store->resource.var->name); + } } return false; diff --git a/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 65ae23eb..2ad11eab 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -25,7 +25,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex; float4 main() : sv_target @@ -34,7 +34,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] // This is allowed in 10.0.10011.16384 but not in 9.29.952.3111 static Texture2D tex; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
On Mon Dec 5 21:14:52 2022 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/54/diffs?diff_id=22975&start_sha=b46edce4737e673a03b634521454a72f40dd4e87#c0def4fdc5564d2e2973931c6613531ebf03ca32_274_267) So, paraphrasing what we talked through IRC with Matteo, 9.29.952.3111 and 10.0.10011.16384 are different versions of fxc. And 9.29.952.3111 uses up to d3dcompiler_43.dll.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18704
On Mon Dec 5 21:14:54 2022 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/54/diffs?diff_id=22975&start_sha=b46edce4737e673a03b634521454a72f40dd4e87#9155b9453b4ec8ea0b9b025dfb55c061bd931610_2180_2179) Oops, I misread. Well, we've already done the harder thing, then :-)
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18706
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
+ + if (load->sampler.var) + { + if (!hlsl_component_index_range_from_deref(ctx, &load->sampler, &start, &count)) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Resource load sampler from \"%s\" must be determinable at compile time.", + load->sampler.var->name); + note_non_static_deref_expressions(ctx, &load->sampler, "resource load sampler"); + } + else if (load->sampler.var->modifiers & HLSL_STORAGE_STATIC) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Loaded sampler from static variable \"%s\" must be initialized.", + load->sampler.var->name); + } Hmm, is this the right condition, though? Should we be checking for !HLSL_STORAGE_UNIFORM instead? Since we want to fail in almost exactly the same way for uninitialized function-scoped variables.
It's perhaps worth mentioning that as written, this will also catch static (or function-scoped) variables that have a non-uniform initialization. IIRC that's forbidden in native too, but it maybe means the compiler message should be adjusted? Like "loads from resource objects must have a single uniform source"? Or maybe we should run liveness analysis before this and check var->last_write. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18707
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
if (type_has_object_components(var->data_type, false)) { - hlsl_fixme(ctx, &var->loc, "Uninitialized static objects."); vkd3d_free(v); continue; }
Is it legal to declare a static texture and initialize it? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18708
On Mon Dec 5 21:52:51 2022 +0000, Zebediah Figura wrote:
Is it legal to declare a static texture and initialize it? Yes, it is legal to declare and initialize a static texture (or more generally, a static variable that contains objects), like in the following shader:
Texture2D real_tex;
static Texture2D tex = real_tex;
float4 main() : sv_target
{
return tex.Load(int3(0, 0, 0));
}
However, that particular piece of code is only active when the variable is **not** initialized, which is legal, as long it is not used before being assigned a proper object (that can be determined statically), such as in this shader: ```hlsl Texture2D real_tex; static Texture2D tex; float4 main() : sv_target { tex = real_tex; return tex.Load(int3(0, 0, 0)); } ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18718
On Mon Dec 5 21:52:51 2022 +0000, Zebediah Figura wrote:
Hmm, is this the right condition, though? Should we be checking for !HLSL_STORAGE_UNIFORM instead? Since we want to fail in almost exactly the same way for uninitialized function-scoped variables. It's perhaps worth mentioning that as written, this will also catch static (or function-scoped) variables that have a non-uniform initialization. IIRC that's forbidden in native too, but it maybe means the compiler message should be adjusted? Like "loads from resource objects must have a single uniform source"? Or maybe we should run liveness analysis before this and check var->last_write.
Hmm, is this the right condition, though? Should we be checking for !HLSL_STORAGE_UNIFORM instead? Since we want to fail in almost exactly the same way for uninitialized function-scoped variables.
I think this is a nice idea. We already have checks for these cases in hlsl_sm4.c: ``` hlsl_fixme(ctx, &load->node.loc, "Load from non-uniform resource variable."); ``` and ```c hlsl_fixme(ctx, &instr->loc, "Object copy."); ``` but those are FIXME·s instead of proper hlsl_error·s.
It's perhaps worth mentioning that as written, this will also catch static (or function-scoped) variables that have a non-uniform initialization. IIRC that's forbidden in native too, but it maybe means the compiler message should be adjusted? Like "loads from resource objects must have a single uniform source"?
Oh, I see. I will change the error messages too.
Or maybe we should run liveness analysis before this and check var->last_write.
I mentioned that it is a TODO to perform this validation after compute_liveness/dce in the message of the second commit. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18771
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)