[PATCH v6 0/5] 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>' ``` -- v6: vkd3d-shader/hlsl: Allow uninitialized static objects. vkd3d-shader/hlsl: Validate that non-uniform objects are not referenced. tests: Add additional object references tests. 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 | 35 +++++++++++++++++++++++ tests/hlsl-static-initializer.shader_test | 9 ++++++ 2 files changed, 44 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index eedc85bd..2e6ef523 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_store *store; + struct hlsl_ir_node *cast; + + /* 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 (!(cast = add_cast(ctx, &ctx->static_initializers, &zero->node, var->data_type, &var->loc))) + { + vkd3d_free(v); + continue; + } + + if (!(store = hlsl_new_simple_store(ctx, var, cast))) + { + vkd3d_free(v); + continue; + } + list_add_tail(&ctx->static_initializers, &store->node.entry); + } 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..47d45f14 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -14,3 +14,12 @@ 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; +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- tests/hlsl-static-initializer.shader_test | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 47d45f14..79a57f3c 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -23,3 +23,33 @@ float4 main() : sv_target { return 1 / i; } + + +[pixel shader] +static struct +{ + float4 x; + float4 y; +} x; + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader] +static struct +{ + float4 x; + float3 y; +} x; + +float4 main() : sv_target +{ + return float4(1, 2, 3, 4) + x.x; +} + +[test] +draw quad +probe all rgba (1.0, 2.0, 3.0, 4.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl-static-initializer.shader_test | 136 ++++++++++++++++++++++ tests/object-references.shader_test | 9 ++ tests/shader_runner_d3d12.c | 2 +- 3 files changed, 146 insertions(+), 1 deletion(-) diff --git a/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 79a57f3c..a1556c1d 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -53,3 +53,139 @@ float4 main() : sv_target [test] draw quad probe all rgba (1.0, 2.0, 3.0, 4.0) + + +[sampler 0] +filter linear linear linear +address clamp clamp clamp + + +[texture 0] +size (1, 1) +1.0 2.0 3.0 4.0 + + +[pixel shader fail todo] +static Texture2D tex; +sampler sam; + +float4 main() : sv_target +{ + return tex.Sample(sam, 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; +sampler sam; + +float4 main() : sv_target +{ + if (0) + return tex.Sample(sam, int3(0, 0, 0)); + else + return float4(0, 1, 2, 3); +} + + +[pixel shader todo] +// This is allowed in 10.0.10011.16384 but not in 9.29.952.3111 +static Texture2D tex; +sampler sam; +uniform uint i; + +float4 main() : sv_target +{ + float4 unused = tex.Sample(sam, int3(0, 1, 2)); + + return 0; +} + + +[pixel shader fail todo] +static Texture2D tex1; +sampler sam; + +float4 main() : sv_target +{ + static Texture2D tex2 = tex1; + return tex2.Sample(sam, int3(0, 0, 0)); +} + + +[pixel shader fail todo] +static Texture2D tex1; +sampler sam; + +float4 main(Texture2D tex2) : sv_target +{ + tex2 = tex1; + return tex2.Sample(sam, int3(0, 0, 0)); +} + + +[pixel shader] +Texture2D real_tex; +static Texture2D tex = real_tex; +sampler sam; + +float4 main() : sv_target +{ + return tex.Sample(sam, int3(0, 0, 0)); +} + +[test] +draw quad +probe all rgba (1, 2, 3, 4) + + +[pixel shader todo] +Texture2D real_tex; +static Texture2D tex; +sampler sam; + +float4 main() : sv_target +{ + tex = real_tex; + return tex.Sample(sam, int3(0, 0, 0)); +} + +[test] +todo draw quad +todo probe all rgba (1, 2, 3, 4) + + +[require] +shader model >= 5.0 + + +[uav 1] +format r32 float +size (1, 1) + +0.5 + + +[pixel shader todo] +// This is allowed in 10.0.10011.16384 but not in 9.29.952.3111 +static RWTexture2D<float> tex; + +float4 main() : sv_target +{ + if (0) + { + tex[int2(0, 0)] = 2; + } + return 0; +} diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 12f745e6..e33a7f38 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -155,3 +155,12 @@ float4 main() : sv_target uniform 0 float 10.0 todo draw quad todo probe (0, 0) rgba (11.0, 12.0, 13.0, 11.0) + + +[pixel shader fail todo] +float4 main(Texture2D tex2) : sv_target +{ + Texture2D tex1; + tex2 = tex1; + return tex2.Load(int3(0, 0, 0)); +} diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index bb4d9c5a..bd94b4c9 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -167,7 +167,7 @@ static ID3D12RootSignature *d3d12_runner_create_root_signature(struct d3d12_shad ID3D12GraphicsCommandList *command_list, unsigned int *uniform_index) { D3D12_ROOT_SIGNATURE_DESC root_signature_desc = {0}; - D3D12_ROOT_PARAMETER root_params[3], *root_param; + D3D12_ROOT_PARAMETER root_params[4], *root_param; D3D12_STATIC_SAMPLER_DESC static_samplers[1]; ID3D12RootSignature *root_signature; HRESULT hr; -- 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 pruning branches, because shaders such as these compile (at least in more modern versions of the native compiler): Branch pruning: ``` static RWTexture2D<float> tex; float4 main() : sv_target { if (0) { tex[int2(0, 0)] = 2; } return 0; } ``` DCE: ``` static Texture2D tex; uniform uint i; float4 main() : sv_target { float4 unused = tex.Load(int3(0, 1, 2)); return 0; } ``` These are "todo" tests in hlsl-static-initializer.shader_test that depend on this. --- libs/vkd3d-shader/hlsl_codegen.c | 33 +++++++++++++++++++++++------ tests/object-references.shader_test | 2 +- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6e4168fc..076902f4 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1009,26 +1009,45 @@ static bool validate_static_object_references(struct hlsl_ctx *ctx, struct hlsl_ { struct hlsl_ir_resource_load *load = hlsl_ir_resource_load(instr); - if (!hlsl_component_index_range_from_deref(ctx, &load->resource, &start, &count)) + if (!(load->resource.var->modifiers & HLSL_STORAGE_UNIFORM)) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Loaded resource must have a single uniform source."); + } + else if (!hlsl_component_index_range_from_deref(ctx, &load->resource, &start, &count)) { hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, "Loaded resource from \"%s\" must be determinable at compile time.", 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)) + + if (load->sampler.var) { - 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"); + if (!(load->sampler.var->modifiers & HLSL_STORAGE_UNIFORM)) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Resource load sampler must have a single uniform source."); + } + else 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 (instr->type == HLSL_IR_RESOURCE_STORE) { struct hlsl_ir_resource_store *store = hlsl_ir_resource_store(instr); - if (!hlsl_component_index_range_from_deref(ctx, &store->resource, &start, &count)) + if (!(store->resource.var->modifiers & HLSL_STORAGE_UNIFORM)) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, + "Accessed resource must have a single uniform source."); + } + else if (!hlsl_component_index_range_from_deref(ctx, &store->resource, &start, &count)) { hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF, "Accessed resource from \"%s\" must be determinable at compile time.", diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index e33a7f38..67f86bc6 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -157,7 +157,7 @@ todo draw quad todo probe (0, 0) rgba (11.0, 12.0, 13.0, 11.0) -[pixel shader fail todo] +[pixel shader fail] float4 main(Texture2D tex2) : sv_target { Texture2D tex1; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
From: Francisco Casas <fcasas(a)codeweavers.com> validate_static_object_references() validates that uninitialized static objects are not referenced in the shader. In case a static variable contains both numeric and object types, the "Static variables cannot have both numeric and resource components." error should preempt uninitialized numeric values to reach further compilation steps. --- libs/vkd3d-shader/hlsl.y | 1 - tests/hlsl-static-initializer.shader_test | 14 +++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 2e6ef523..3f3f80c8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2177,7 +2177,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/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index a1556c1d..74ca7410 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -65,7 +65,7 @@ size (1, 1) 1.0 2.0 3.0 4.0 -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex; sampler sam; @@ -75,7 +75,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; @@ -113,7 +113,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex1; sampler sam; @@ -124,7 +124,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex1; sampler sam; @@ -150,7 +150,7 @@ draw quad probe all rgba (1, 2, 3, 4) -[pixel shader todo] +[pixel shader] Texture2D real_tex; static Texture2D tex; sampler sam; @@ -162,8 +162,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (1, 2, 3, 4) +draw quad +probe all rgba (1, 2, 3, 4) [require] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
On Wed Dec 7 18:41:39 2022 +0000, Zebediah Figura wrote:
I think that's a good idea, yes. Zeb, do you know the quick way to access that information programmatically?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19558
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
On Tue Dec 13 11:27:51 2022 +0000, Giovanni Mascellani wrote:
Zeb, do you know the quick way to access that information programmatically? get_file_version() in programs/winetest/main.c is probably a good example.
If I understand correctly, though, in this case it's a difference between 43 and 47? Do we know if there's actually any differences between 47 builds? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19609
On Wed Dec 14 04:05:39 2022 +0000, Zebediah Figura wrote:
get_file_version() in programs/winetest/main.c is probably a good example. If I understand correctly, though, in this case it's a difference between 43 and 47? Do we know if there's actually any differences between 47 builds? At some point I thought so, but I am not so sure any more.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19630
I realized that this shader makes the compiler to get stuck in the copy-prop loop: ```c struct apple { Texture2D tex; float4 fo : COLOR; }; float4 main(struct apple input) : sv_target { input.tex = input.tex; // assignment to itself return input.tex.Load(int3(0, 0, 0)); } ``` There may be a more general problem, I am investigating this. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_20324
On Wed Jan 4 18:01:39 2023 +0000, Francisco Casas wrote:
I realized that this shader makes the compiler to get stuck in the copy-prop loop: ```c struct apple { Texture2D tex; float4 fo : COLOR; }; float4 main(struct apple input) : sv_target { input.tex = input.tex; // assignment to itself return input.tex.Load(int3(0, 0, 0)); } ``` There may be a more general problem, I am investigating this. This currently also happens on master, so it is not a problem of this MR.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_20325
On Wed Jan 4 18:01:39 2023 +0000, Francisco Casas wrote:
This currently also happens on master, so it is not a problem of this MR. !59 solves this issue.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_20335
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
Like !47, this conflicts with !50. Specifically, the rename of hlsl_ir_var.modifiers. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_20682
participants (6)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)