[PATCH 0/1] MR239: vkd3d-shader/codegen: Skip register allocation for unused object variables.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 2 +- tests/register-reservations.shader_test | 31 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 96b4cb660..829e11c34 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -3658,7 +3658,7 @@ static void allocate_objects(struct hlsl_ctx *ctx, enum hlsl_regset regset) { unsigned int count = var->regs[regset].bind_count; - if (count == 0) + if (count == 0 || !var->last_read) continue; if (var->regs[regset].allocated) diff --git a/tests/register-reservations.shader_test b/tests/register-reservations.shader_test index 72f68c1e0..630540149 100644 --- a/tests/register-reservations.shader_test +++ b/tests/register-reservations.shader_test @@ -81,3 +81,34 @@ float4 main() : sv_target [test] draw quad probe all rgba (4.0, 4.0, 4.0, 99.0) + + +% Overlapping reservations, both overllaping objects are unused +[pixel shader] +Texture2D tex1 : register(t0); +Texture2D tex2 : register(t0); +Texture2D tex3; + +float4 main() : sv_target +{ + return tex3.Load(int3(0, 0, 0)); +} + +[test] +draw quad +probe all rgba (1.0, 1.0, 1.0, 99.0) + + +% Overlapping reservations +[pixel shader] +Texture2D tex1 : register(t2); +Texture2D tex2 : register(t2); + +float4 main() : sv_target +{ + return tex1.Load(int3(0, 0, 0)); +} + +[test] +draw quad +probe all rgba (2.0, 2.0, 2.0, 99.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239
Most of the code assumes that if "allocated" is set then the variable is used. It seems like an error that allocate_register_reservations() is setting it even for unused variables. Francisco, is there a reason we're doing that? Why does get_allocated_object() check that flag? FWIW, I'm pretty sure this is also a regression, although I don't know from when. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239#note_36149
Most of the code assumes that if "allocated" is set then the variable is used. It seems like an error that allocate_register_reservations() is setting it even for unused variables.
I think you are right. I probably introduced this regression in f8338ef08953686298bb31315a19e6044325bb49, I forgot to skip unused objects in allocate_register_reservations() when I wrote it. Yes, probably the best solution is to add ```c if (!var->last_read || !var->data_type->reg_size[regset]) continue; ``` in allocate_register_reservations().
Francisco, is there a reason we're doing that? Why does get_allocated_object() check that flag?
Once that flag is set, the object registers are assumed to be reserved for that variable. allocate_register_reservations() should ensure that flag is set for all **used objects with register reservations** (we clearly have to fix the **used** part), and also allocate_objects() sets it for objects without register reservations, one at the time. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239#note_36199
On Tue Jun 20 00:50:34 2023 +0000, Francisco Casas wrote:
Most of the code assumes that if "allocated" is set then the variable is used. It seems like an error that allocate_register_reservations() is setting it even for unused variables. ~~I think you are right. I probably introduced this regression in f8338ef08953686298bb31315a19e6044325bb49, I forgot to skip unused objects in allocate_register_reservations() when I wrote it.~~ ~~Yes, probably the best solution is to add~~
if (!var->last_read || !var->data_type->reg_size[regset]) continue;~~in allocate_register_reservations().~~
Francisco, is there a reason we're doing that? Why does get_allocated_object() check that flag? ~~Once that flag is set, the object registers are assumed to be reserved for that variable. allocate_register_reservations() should ensure that flag is set for all **used objects with register reservations** (we clearly have to fix the **used** part), and also allocate_objects() sets it for objects without register reservations, one at the time.~~ No, wait. That's not it. Ignore my previous comment. There is even a test that says:
% Register reservations force to reserve all the resource registers. Even if unused.
I see now that what we ought to do is ignore overlaps when one of the objects with register reservations is not used. So this MR is correct regarding that. Ugh, I will answer again then:
Francisco, is there a reason we're doing that? Why does get_allocated_object() check that flag?
We have to make sure that those spaces are reserved, even if the object is not used, like in the test with the previous comment. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239#note_36202
participants (4)
-
Francisco Casas (＠fcasas) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)