[PATCH v6 0/3] 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. -- v6: vkd3d-shader/hlsl: Validate that referenced objects are uniform. tests: Test uninitialized object references. vkd3d-shader/hlsl: Avoid infinite loop in copy propagation. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
From: Francisco Casas <fcasas(a)codeweavers.com> 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. --- libs/vkd3d-shader/hlsl.c | 27 +++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl_codegen.c | 3 +++ tests/object-references.shader_test | 37 +++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index b4f705da..6c3bdfac 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -846,6 +846,33 @@ void hlsl_cleanup_deref(struct hlsl_deref *deref) hlsl_src_remove(&deref->offset); } +bool hlsl_derefs_are_equal(struct hlsl_ctx *ctx, const struct hlsl_deref *deref1, + const struct hlsl_deref *deref2) +{ + unsigned int i; + + assert(!deref1->offset.node); + assert(!deref2->offset.node); + + if (deref1->var != deref2->var) + return false; + if (deref1->path_len != deref2->path_len) + return false; + + for (i = 0; i < deref1->path_len; ++i) + { + const struct hlsl_ir_node *node1 = deref1->path[i].node, *node2 = deref2->path[i].node; + + if (node1 == node2) + continue; + if (node1->type != HLSL_IR_CONSTANT || node2->type != HLSL_IR_CONSTANT) + return false; + if (hlsl_ir_constant(node1)->value[0].u != hlsl_ir_constant(node2)->value[0].u) + return false; + } + return true; +} + /* Initializes a simple variable derefence, so that it can be passed to load/store functions. */ void hlsl_init_simple_deref_from_var(struct hlsl_deref *deref, struct hlsl_ir_var *var) { diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index f365f588..f5148de4 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -890,6 +890,8 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry bool hlsl_copy_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, const struct hlsl_deref *other); void hlsl_cleanup_deref(struct hlsl_deref *deref); +bool hlsl_derefs_are_equal(struct hlsl_ctx *ctx, const struct hlsl_deref *deref1, + const struct hlsl_deref *deref2); void hlsl_replace_node(struct hlsl_ir_node *old, struct hlsl_ir_node *new); diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index d21e31be..e4173ddc 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -773,6 +773,9 @@ static bool copy_propagation_transform_object_load(struct hlsl_ctx *ctx, /* Only HLSL_IR_LOAD can produce an object. */ load = hlsl_ir_load(instr); + if (hlsl_derefs_are_equal(ctx, deref, &load->src)) + 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 12f745e6..78fabd79 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -1,3 +1,15 @@ +[pixel shader fail todo] +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 @@ -155,3 +167,28 @@ 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) + + +[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
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/object-references.shader_test | 41 +++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 78fabd79..39716e64 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -10,6 +10,47 @@ float4 main() : sv_target } +[pixel shader fail todo] +sampler sam; + +float4 main() : sv_target +{ + Texture2D tex; + + return tex.Sample(sam, float2(0, 0)); +} + + +[pixel shader fail todo] +Texture2D tex; + +float4 main() : sv_target +{ + sampler sam; + + return tex.Sample(sam, float2(0, 0)); +} + + +[pixel shader fail todo] +float4 main() : sv_target +{ + RWTexture2D<float4> u; + + u[uint2(0, 0)] = 1; + return 0; +} + + +[pixel shader fail todo] +float4 main() : sv_target +{ + RWTexture2D<float4> u; + + return u[uint2(0, 0)]; +} + + [require] shader model >= 4.0 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 32 ++++++++++++++++++------ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + tests/object-references.shader_test | 10 ++++---- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index e4173ddc..2d38a272 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1012,26 +1012,44 @@ 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->is_uniform) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_UNINITIALIZED_OBJECT_REF, + "Loaded resource not initialized."); + } + 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->is_uniform) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_UNINITIALIZED_OBJECT_REF, + "Resource load sampler not initialized."); + } + 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->is_uniform) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_UNINITIALIZED_OBJECT_REF, + "Accessed resource not initialized."); + } + 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/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 74edf049..e2f14ea3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -121,6 +121,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_HLSL_NON_STATIC_OBJECT_REF = 5022, VKD3D_SHADER_ERROR_HLSL_INVALID_THREAD_COUNT = 5023, VKD3D_SHADER_ERROR_HLSL_MISSING_ATTRIBUTE = 5024, + VKD3D_SHADER_ERROR_HLSL_UNINITIALIZED_OBJECT_REF = 5025, VKD3D_SHADER_WARNING_HLSL_IMPLICIT_TRUNCATION = 5300, VKD3D_SHADER_WARNING_HLSL_DIVISION_BY_ZERO = 5301, diff --git a/tests/object-references.shader_test b/tests/object-references.shader_test index 39716e64..50dd52f2 100644 --- a/tests/object-references.shader_test +++ b/tests/object-references.shader_test @@ -1,4 +1,4 @@ -[pixel shader fail todo] +[pixel shader fail] sampler sam; float4 main() : sv_target @@ -10,7 +10,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] sampler sam; float4 main() : sv_target @@ -21,7 +21,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] Texture2D tex; float4 main() : sv_target @@ -32,7 +32,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] float4 main() : sv_target { RWTexture2D<float4> u; @@ -42,7 +42,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] float4 main() : sv_target { RWTexture2D<float4> u; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
:arrow_up: Rebased on top of master. It is worth noting that the last patch currently conflicts with !54's patch ``` vkd3d-shader/hlsl: Validate that non-uniform objects are not referenced. ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20767
Update: As we discussed on IRC, the bug that this MR tries to solve is a symptom of a more severe underlying problem in copy-propagation, specifically, the use and replace of `hlsl_deref`s in `copy_propagation_transform_object_load()`. Zeb pointed out this problem and suggested the following example (with `a`, `b`, and `c` objects): ``` a = b; b = c; ret = Tex2D(a); ``` were if `ret = Tex2D(a);` gets replaced into `ret = Tex2D(b);` before copy-prop replaces `b` with its uniform copy, it could happen that the third line ends up as `text2D(c)`. This actually happens using the master branch to compile the following shader: ```hlsl Texture2D t_good, t_bad; 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; return a.Load(int3(0, 0, 0)); } ``` were `t_bad` is loaded instead of `t_good`. So probably Giovanni's solution, with some modifications, is the correct solution (as comparing deref shouldn't be solving this underlying issue). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_21280
participants (2)
-
Francisco Casas -
Francisco Casas (＠fcasas)