[PATCH v3 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. -- v3: 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> Co-authored-by: Giovanni Mascellani <gmascellani(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 preempts this by avoiding propagating object loads when the variable is a real object (i.e., a uniform variable) which cannot have a replacement. --- libs/vkd3d-shader/hlsl_codegen.c | 6 +++++ tests/object-references.shader_test | 37 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6e4168fc..037c241c 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -773,6 +773,12 @@ 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 (!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 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/hlsl_sm4.c | 18 +++---------- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + tests/object-references.shader_test | 10 ++++---- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 037c241c..3df18cdb 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1015,26 +1015,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/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index ae5bb1ac..cbf7b007 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -2155,18 +2155,10 @@ static void write_sm4_resource_load(struct hlsl_ctx *ctx, assert(sampler_type->base_type == HLSL_TYPE_SAMPLER); assert(sampler_type->sampler_dim == HLSL_SAMPLER_DIM_GENERIC); - if (!load->sampler.var->is_uniform) - { - hlsl_fixme(ctx, &load->node.loc, "Sample using non-uniform sampler variable."); - return; - } + assert(load->sampler.var->is_uniform); } - if (!load->resource.var->is_uniform) - { - hlsl_fixme(ctx, &load->node.loc, "Load from non-uniform resource variable."); - return; - } + assert(load->resource.var->is_uniform); switch (load->load_type) { @@ -2219,11 +2211,7 @@ static void write_sm4_resource_store(struct hlsl_ctx *ctx, return; } - if (!store->resource.var->is_uniform) - { - hlsl_fixme(ctx, &store->node.loc, "Store to non-uniform resource variable."); - return; - } + assert(store->resource.var->is_uniform); write_sm4_store_uav_typed(ctx, buffer, &store->resource, store->coords.node, store->value.node); } 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: The new version avoids infinite loops using Giovanni's solution, and also validates that object references are uniform. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20397
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59
Code looks fine, though your commit message says "This patch preempts this by avoiding propagating object loads when the variable is a real object (i.e., a uniform variable) which cannot have a replacement", while unless I am missing something the opposite is true: object loads are propagated only when coming from an uniform object. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20561
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)