[PATCH v3 0/4] 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>' ``` -- v3: vkd3d-shader/hlsl: Allow uninitialized static objects. vkd3d-shader/hlsl: Validate that non-uniform objects are not referenced. tests: Add additional object references tests. 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 | 9 ++++++ 2 files changed, 43 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..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: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl-static-initializer.shader_test | 88 +++++++++++++++++++++++ tests/object-references.shader_test | 9 +++ tests/shader_runner_d3d12.c | 2 +- 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 47d45f14..34cb8864 100644 --- a/tests/hlsl-static-initializer.shader_test +++ b/tests/hlsl-static-initializer.shader_test @@ -23,3 +23,91 @@ 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); +} + + +[pixel shader fail todo] +static Texture2D tex1; + +float4 main() : sv_target +{ + static Texture2D tex2 = tex1; + return tex2.Load(int3(0, 0, 0)); +} + + +[pixel shader fail todo] +static Texture2D tex1; + +float4 main(Texture2D tex2) : sv_target +{ + tex2 = tex1; + return tex2.Load(int3(0, 0, 0)); +} + + +[texture 0] +size (1, 1) +1.0 2.0 3.0 4.0 + + +[pixel shader] +Texture2D real_tex; +static Texture2D tex = real_tex; + +float4 main() : sv_target +{ + return tex.Load(int3(0, 0, 0)); +} + + +[test] +draw quad +probe all rgba (1, 2, 3, 4) + + +[pixel shader todo] +Texture2D real_tex; +static Texture2D tex; + +float4 main() : sv_target +{ + tex = real_tex; + return tex.Load(int3(0, 0, 0)); +} + + +[test] +todo draw quad +todo probe all rgba (1, 2, 3, 4) 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 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_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..cf437cd1 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, + "Accesed 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. --- 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 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/tests/hlsl-static-initializer.shader_test b/tests/hlsl-static-initializer.shader_test index 34cb8864..7cdc33be 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; @@ -57,7 +57,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex1; float4 main() : sv_target @@ -67,7 +67,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] static Texture2D tex1; float4 main(Texture2D tex2) : sv_target @@ -97,7 +97,7 @@ draw quad probe all rgba (1, 2, 3, 4) -[pixel shader todo] +[pixel shader] Texture2D real_tex; static Texture2D tex; @@ -109,5 +109,5 @@ 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) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54
Running the latest tests on native `d3dcompiler_47.dll` (which `wrestool -x --raw --type=16` describes as version "10.0.15063.674 (WinBuild.160101.0800)"), there are still a couple of failures in `hlsl-static-initializer.shader_test`: ``` shader_runner:65:Section [test], line 95: Test failed: Failed to compile shader, hr 0x80004001. shader_runner:69:Section [test], line 95: Z:\home\giovanni\progetti\windows\wine\build64\Shader(a)0x00000000022B60F0(5,12-34): error X4532: cannot map expression to pixel shader instruction set shader_runner:65:Section [test], line 111: Test failed: Failed to compile shader, hr 0x80004001. shader_runner:69:Section [test], line 111: Z:\home\giovanni\progetti\windows\wine\build64\Shader(a)0x00000000022B61F0(6,12-34): error X4532: cannot map expression to pixel shader instruction set ``` I am not really sure of what the error message really means. Maybe in this case it makes sense to just pretend that native is buggy and ignore it, but let's at least have a chance to discuss that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18848
On Mon Dec 5 21:14:53 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_270_267) That makes sense, but once the behavior is fixed the test is deterministic and you can add it.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18855
On Mon Dec 5 21:26:43 2022 +0000, Francisco Casas wrote:
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. Maybe we should modify the shader runner so that it dumps the d3dcompiler version it is using, at least when it's using the native library. Not for this MR, though, just thinking loudly.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18856
On Tue Dec 6 16:19:46 2022 +0000, Giovanni Mascellani wrote:
Running the latest tests on native `d3dcompiler_47.dll` (which `wrestool -x --raw --type=16` describes as version "10.0.15063.674 (WinBuild.160101.0800)"), there are still a couple of failures in `hlsl-static-initializer.shader_test`: ``` shader_runner:65:Section [test], line 95: Test failed: Failed to compile shader, hr 0x80004001. shader_runner:69:Section [test], line 95: Z:\home\giovanni\progetti\windows\wine\build64\Shader(a)0x00000000022B60F0(5,12-34): error X4532: cannot map expression to pixel shader instruction set shader_runner:65:Section [test], line 111: Test failed: Failed to compile shader, hr 0x80004001. shader_runner:69:Section [test], line 111: Z:\home\giovanni\progetti\windows\wine\build64\Shader(a)0x00000000022B61F0(6,12-34): error X4532: cannot map expression to pixel shader instruction set ``` I am not really sure of what the error message really means. Maybe in this case it makes sense to just pretend that native is buggy and ignore it, but let's at least have a chance to discuss that. I have seen that error when the shader is compiled using a target profile that doesn't have the required features. Which target profile are you using to compile? I am using ps_5_0.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18860
On Tue Dec 6 16:19:46 2022 +0000, Francisco Casas wrote:
I have seen that error when the shader is compiled using a target profile that doesn't have the required features. Which target profile are you using to compile? I am using ps_5_0. I checked with a newer version of the native compiler and "10.0.19041.868 (WinBuild.160101.0800)" and I don't see a different behavior with the tests.
btw, did you check that your version of the native compiler is indeed loading that dll with `WINEDEBUG=+loaddll` (note that there is one in `windows/system32` and other in `windows/syswow64`). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_18920
On Tue Dec 6 23:50:17 2022 +0000, Francisco Casas wrote:
I checked with a newer version of the native compiler and "10.0.19041.868 (WinBuild.160101.0800)" and I don't see a different behavior with the tests. btw, did you check that your version of the native compiler is indeed loading that dll with `WINEDEBUG=+loaddll` (note that there is one in `windows/system32` and other in `windows/syswow64`). I don't know what happened yesterday, but today the tests work with both 10.0.15063.674 and 10.0.19041.868. Sorry for the noise!
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19013
This is accepted by native, but fails to compile with ours: ```hlsl static struct { float4 x; float4 y; } x; float4 main() : sv_target { return 0; } ``` The error is: ``` shader_runner:684:Section [pixel shader], line 115: Test failed: Got unexpected hr 0x80004005. shader_runner:697:Section [pixel shader], line 115: <anonymous>:5:3: E5002: Can't implicitly convert from uint to <anonymous struct>. ``` It's ok if you consider this out of scope for this MR, but please add a test showing the todo. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19020
On Tue Dec 6 16:03:41 2022 +0000, Giovanni Mascellani wrote:
Maybe we should modify the shader runner so that it dumps the d3dcompiler version it is using, at least when it's using the native library. Not for this MR, though, just thinking loudly. I think that's a good idea, yes.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19047
It's ok if you consider this out of scope for this MR, but please add a test showing the todo.
Actually, this seems like it probably should be fixed in these patches. We have scalar-to-struct casts; this should work already. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19048
On Wed Dec 7 18:43:17 2022 +0000, Zebediah Figura wrote:
It's ok if you consider this out of scope for this MR, but please add a test showing the todo. Actually, this seems like it probably should be fixed in these patches. We have scalar-to-struct casts; this should work already. Ah, I see now.
Broadcasts aren't allowed as implicit conversions by either the native compiler or our compiler, and I am using an assignment here which only does an implicit conversion, but I should use an explicit conversion which is more permissive. I will add this test too. Thanks @giomasce for pointing this out! -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/54#note_19182
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)