[PATCH v8 0/2] MR59: vkd3d-shader/hlsl: Avoid infinite loop in copy propagation.
copy_propagation_transform_object_load() currently retrieves true, which indicates that there was progress, even if the input dereference remains the same. This can happen repeatedly if an uninitialized object is copied onto itself, as in the tests added in this patch. This results on the compilation getting stuck on an endless loop. This patch checks if the deref didn't change, to retrieve false in that case. -- v8: vkd3d-shader/hlsl: Avoid infinite loop and invalid derefs in copy-prop. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/object-references.shader_test | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 5e8e2641..0c203d5a 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -1,6 +1,43 @@ [require] shader model >= 4.0 + +[texture 0] +size (1, 1) +0.77 0.77 0.77 0.77 + +[texture 1] +size (1, 1) +0.64 0.64 0.64 0.64 + +[sampler 0] +filter linear linear linear +address clamp clamp clamp + +[pixel shader] +Texture2D t_good, t_bad; +sampler sam; + +float4 main() : sv_target +{ + Texture2D a, b[1]; + + // This is basically a 'b[0] = t_good' but so that the copy-prop is delayed + int4 co = {0, 0, 0, 0}; + b[(int) co.x + (int) co.y] = t_good; + + a = b[0]; + b[0] = t_bad; + + // 'a' should be 't_good', not 't_bad' at this point + return 100 * a.Sample(sam, float2(0, 0)) + t_good.Sample(sam, float2(0, 0)); +} + +[test] +draw quad +todo probe all rgba (77.77, 77.77, 77.77, 77.77) + + [texture 0] size (2, 2) 0.1 0.2 0.3 0.4 0.5 0.7 0.6 0.8 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Co-authored-by: Francisco Casas <fcasas(a)codeweavers.com> Co-authored-by: Zebediah Figura <zfigura(a)codeweavers.com> Because copy_propagation_transform_object_load() replaces a deref instead of an instruction, it is currently prone to two problems: 1- It can replace a deref with the same deref, returning true every time and getting the compilation stuck in an endless loop of copy-propagation iterations. 2- When performed multiple times in the same deref, the second time it can replace the deref with a deref from a temp that is only valid in another point of the program execution, resulting in an incorrect value. This patch preempts this by avoiding replacing derefs when the new deref doesn't point to a uniform variable. Because, uniform variables cannot be written to. --- libs/vkd3d-shader/hlsl_codegen.c | 20 +++++++++++++++ tests/object-references.shader_test | 39 ++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 5e7cfff4..07ad8858 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -927,6 +927,26 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx, /* Only HLSL_IR_LOAD can produce an object. */ load = hlsl_ir_load(value->node); + /* As we are replacing the instruction's deref (with the one in the hlsl_ir_load) and not the + * instruction itself, we won't be able to rely on the value retrieved by + * copy_propagation_get_value() for the new deref in subsequent iterations of copy propagation. + * This is because another value may be written to that deref between the hlsl_ir_load and + * this instruction. + * + * For this reason, we only replace the new deref when it corresponds to a uniform variable, + * which cannot be written to. + * + * In a valid shader, all object references must resolve statically to a single uniform object. + * If this is the case, we can expect copy propagation on regular store/loads and the other + * compilation passes to replace all hlsl_ir_loads with loads to uniform objects, so this + * implementation is complete, even with this restriction. + */ + if (!load->src.var->is_uniform) + { + TRACE("Ignoring load from non-uniform object variable %s\n", load->src.var->name); + return false; + } + hlsl_cleanup_deref(deref); hlsl_copy_deref(ctx, deref, &load->src); diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 0c203d5a..bc74ccd4 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -1,3 +1,15 @@ +[pixel shader fail] +sampler sam; + +float4 main() : sv_target +{ + Texture2D tex; + + tex = tex; // Uninitialized assignment to self. + return tex.Sample(sam, float2(0, 0)); +} + + [require] shader model >= 4.0 @@ -35,7 +47,7 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (77.77, 77.77, 77.77, 77.77) +probe all rgba (77.77, 77.77, 77.77, 77.77) [texture 0] @@ -201,3 +213,28 @@ float4 main(Texture2D tex2) : sv_target tex2 = tex1; return tex2.Load(int3(0, 0, 0)); } + + +[require] +shader model >= 5.0 + + +[texture 0] +size (1, 1) +1.0 2.0 3.0 4.0 + +[pixel shader todo] +struct apple { + Texture2D tex; + float4 fo : COLOR; +}; + +float4 main(struct apple input) : sv_target +{ + input.tex = input.tex; // Assignment to self. + return input.tex.Load(int3(0, 0, 0)); +} + +[test] +todo draw quad +todo probe (0, 0) rgba (1.0, 2.0, 3.0, 4.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
:arrow_up: Added explanatory comment. Does it look good? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_21835
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani