[PATCH 0/1] MR239: vkd3d-shader/codegen: Skip register allocation for unused object variables.

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov

[PATCH 1/1] vkd3d-shader/codegen: Skip register allocation for unused object variables.

[Nikolay Sivov]

From: Nikolay Sivov

Signed-off-by: Nikolay Sivov 
---
 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)

[Zebediah Figura (＠zfigura)]

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.

[Francisco Casas (＠fcasas)]

> 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.

[Francisco Casas (＠fcasas)]

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.