[PATCH v4 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. -- v4: 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 only propagating object loads when the variable is a real object (i.e., a uniform variable) which cannot have a replacement, since they cannot be stored to. --- 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
On Mon Jan 9 12:43:12 2023 +0000, Giovanni Mascellani wrote:
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. Oh, right, that is a typo. I just fixed it.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20564
On Thu Jan 5 19:06:48 2023 +0000, Francisco Casas wrote:
This made me realize we probably want to check for the variables being uniform in `validate_static_object_references()`, so that the first test passes and we don't reach: `Aborting due to not yet implemented feature: Load from non-uniform resource variable.` or `Aborting due to not yet implemented feature: Object copy.` and the first tests fails correctly with an `hlsl_error()` instead of an `hlsl_fixme()`. I'm afraid I disagree with this analysis quite strongly. While we *can* do copy-prop this way, there's fundamentally no reason why we can't copy-prop from a temporary object variable, making this code look questionable already. Worse, I think it's a lot less clear why this prevents an infinite loop, compared to Francisco's initial approach.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20597
(2) We're going to need to get rid of these assignments anyway, though. Consider the last test, which still fails—I assume because we're left with object load/stores in the IR and we can't translate those to sm4.
The endless loop seems to happen when there are uninitialized objects such as in the first test (or in the second test for now, because we are lacking more support for object components) because copy-prop indeed can't get rid of these assignments.
Eh, right... I guess the way we do copy-prop we're probably not going to end up running into this case unless the object was never initialized? I don't feel comfortable that's always going to be true, though, and I don't like depending on that in the future. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20599
On Mon Jan 9 22:06:12 2023 +0000, Zebediah Figura wrote:
I'm afraid I disagree with this analysis quite strongly. While we *can* do copy-prop this way, there's fundamentally no reason why we can't copy-prop from a temporary object variable, making this code look questionable already. Worse, I think it's a lot less clear why this prevents an infinite loop, compared to Francisco's initial approach. In my understanding, the fundamental reason why we cannot propagate from a temporary object is that temporary objects do not really exist. They are just a static kind of pointer. The only objects that exist really, AFAIU the language, are those represented by a uniform variable (which is later given a register type and number). All other object variables must be resolved at compilation time to a uniform object, or the program is to be rejected. From this point of view the thing that is meaningful to track is the underlying real uniform object, and once you adopt this point of view the bug just isn't there any more.
Actually, that would probably be an argument for moving the `!is_uniform` check to `copy_propagation_record_store()`. The copy propagation tables themselves should contain no record of a store coming from a temporary object variable, for the reason above. Suspecting that you won't agree with me anyway, I guess you're the one to decide, and I agree that solving the bug in the way I don't like is still better than not solving it at all. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20611
In my understanding, the fundamental reason why we cannot propagate from a temporary object is that temporary objects do not really exist. They are just a static kind of pointer. The only objects that exist really, AFAIU the language, are those represented by a uniform variable (which is later given a register type and number). All other object variables must be resolved at compilation time to a uniform object, or the program is to be rejected. From this point of view the thing that is meaningful to track is the underlying real uniform object, and once you adopt this point of view the bug just isn't there any more.
Well... eh. From my perspective, there's nothing fundamentally wrong with saying that temporary objects exist. We have an obligation to get rid of them all by the time we're emitting SM1 or SM4, but that's just a restriction of the byte code. If we had a format that could deal with temporary objects—like, say, SM6, which we'll probably emit one of these days!—that whole axiom would go out the window.
Suspecting that you won't agree with me anyway, I guess you're the one to decide, and I agree that solving the bug in the way I don't like is still better than not solving it at all.
I'm sorry, I don't mean to frustrate by arguing. Plus, in general, if there's a disagreement I'd rather resolve it before putting my foot down—or, failing that, to at least understand the disagreement, and ideally reduce it down to a stylistic preference. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20651
On Tue Jan 10 23:05:02 2023 +0000, Zebediah Figura wrote:
In my understanding, the fundamental reason why we cannot propagate from a temporary object is that temporary objects do not really exist. They are just a static kind of pointer. The only objects that exist really, AFAIU the language, are those represented by a uniform variable (which is later given a register type and number). All other object variables must be resolved at compilation time to a uniform object, or the program is to be rejected. From this point of view the thing that is meaningful to track is the underlying real uniform object, and once you adopt this point of view the bug just isn't there any more. Well... eh. From my perspective, there's nothing fundamentally wrong with saying that temporary objects exist. We have an obligation to get rid of them all by the time we're emitting SM1 or SM4, but that's just a restriction of the byte code. If we had a format that could deal with temporary objects—like, say, SM6, which we'll probably emit one of these days!—that whole axiom would go out the window. Suspecting that you won't agree with me anyway, I guess you're the one to decide, and I agree that solving the bug in the way I don't like is still better than not solving it at all. I'm sorry, I don't mean to frustrate by arguing. Plus, in general, if there's a disagreement I'd rather resolve it before putting my foot down—or, failing that, to at least understand the disagreement, and ideally reduce it down to a stylistic preference. Alright, the possibility of temporary objects in SM6 argument is good enough.
If it wasn't for it I would be more inclined for Giovanni's approach, albeit, I suspect that adding the `!is_uniform` check on `copy_propagation_record_store()` would preempt some static references to be resolved. I will revert to the previous approach. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/59#note_20692
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)