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

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov

--
  v2: vkd3d-shader/codegen: Skip register allocation for unused object variables.

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/239

[PATCH v2 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        | 52 +++++++++++++++------
 tests/register-reservations.shader_test | 61 +++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
index 96b4cb660..f8511a1f9 100644
--- a/libs/vkd3d-shader/hlsl_codegen.c
+++ b/libs/vkd3d-shader/hlsl_codegen.c
@@ -2688,6 +2688,7 @@ static char get_regset_name(enum hlsl_regset regset)

static void allocate_register_reservations(struct hlsl_ctx *ctx)
 {
+    struct vkd3d_string_buffer *type_string;
     struct hlsl_ir_var *var;

LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
@@ -2696,29 +2697,49 @@ static void allocate_register_reservations(struct hlsl_ctx *ctx)

if (!hlsl_type_is_resource(var->data_type))
             continue;
+        if (!var->reg_reservation.reg_type)
+            continue;
+
         regset = hlsl_type_get_regset(var->data_type);

-        if (var->reg_reservation.reg_type)
+        if (var->reg_reservation.reg_type != get_regset_name(regset))
         {
-            if (var->reg_reservation.reg_type != get_regset_name(regset))
+            if (var->last_read)
             {
-                struct vkd3d_string_buffer *type_string;
-
                 type_string = hlsl_type_to_string(ctx, var->data_type);
                 hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION,
                         "Object of type '%s' must be bound to register type '%c'.",
                         type_string->buffer, get_regset_name(regset));
                 hlsl_release_string_buffer(ctx, type_string);
             }
-            else
-            {
-                var->regs[regset].allocated = true;
-                var->regs[regset].id = var->reg_reservation.reg_index;
-                var->regs[regset].bind_count = var->data_type->reg_size[regset];
-                TRACE("Allocated reserved %s to %c%u-%c%u.\n", var->name, var->reg_reservation.reg_type,
-                        var->reg_reservation.reg_index, var->reg_reservation.reg_type,
-                        var->reg_reservation.reg_index + var->regs[regset].bind_count);
-            }
+            continue;
+        }
+
+        var->regs[regset].allocated = true;
+        var->regs[regset].id = var->reg_reservation.reg_index;
+        var->regs[regset].bind_count = var->data_type->reg_size[regset];
+        TRACE("Allocated reserved %s to %c%u-%c%u.\n", var->name, var->reg_reservation.reg_type,
+                var->reg_reservation.reg_index, var->reg_reservation.reg_type,
+                var->reg_reservation.reg_index + var->regs[regset].bind_count);
+    }
+}
+
+static void deallocate_unused_register_reservations(struct hlsl_ctx *ctx)
+{
+    struct hlsl_ir_var *var;
+
+    LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
+    {
+        enum hlsl_regset regset;
+
+        if (!hlsl_type_is_resource(var->data_type))
+            continue;
+        if (!var->reg_reservation.reg_type)
+            continue;
+        regset = hlsl_type_get_regset(var->data_type);
+        if (var->regs[regset].allocated && !var->last_read)
+        {
+            var->regs[regset].allocated = false;
         }
     }
 }
@@ -3666,6 +3687,9 @@ static void allocate_objects(struct hlsl_ctx *ctx, enum hlsl_regset regset)
             const struct hlsl_ir_var *reserved_object, *last_reported = NULL;
             unsigned int index, i;

+            if (!var->last_read)
+                continue;
+
             if (var->regs[regset].id < min_index)
             {
                 assert(regset == HLSL_REGSET_UAVS);
@@ -4130,6 +4154,8 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry
     allocate_semantic_registers(ctx);
     allocate_objects(ctx, HLSL_REGSET_SAMPLERS);

+    deallocate_unused_register_reservations(ctx);
+
     if (ctx->result)
         return ctx->result;

diff --git a/tests/register-reservations.shader_test b/tests/register-reservations.shader_test
index 72f68c1e0..26f4381d0 100644
--- a/tests/register-reservations.shader_test
+++ b/tests/register-reservations.shader_test
@@ -69,6 +69,36 @@ draw quad
 probe all rgba (1.0, 1.0, 1.0, 99.0)

+% Register reservation with incorrect register type.
+[pixel shader]
+Texture2D unused : register(s0);
+Texture2D tex;
+
+float4 main() : sv_target
+{
+    return tex.Load(int3(0, 0, 0));
+}
+
+[test]
+draw quad
+probe all rgba (0.0, 0.0, 0.0, 99.0)
+
+
+% Register reservation with incorrect register type.
+[pixel shader]
+sampler2D unused : register(t0);
+Texture2D tex;
+
+float4 main() : sv_target
+{
+    return tex.Load(int3(0, 0, 0));
+}
+
+
+[test]
+draw quad
+probe all rgba (0.0, 0.0, 0.0, 99.0)
+
 [pixel shader]
 Texture2D unused[2][2] : register(t0);
 Texture2D tex;
@@ -81,3 +111,34 @@ float4 main() : sv_target
 [test]
 draw quad
 probe all rgba (4.0, 4.0, 4.0, 99.0)
+
+
+% Overlapping reservations, both overlapping 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)

[Nikolay Sivov (＠nsivov)]

On Tue Jun 20 00:50:00 2023 +0000, Francisco Casas wrote:
> 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.

This is hopefully better now, 'allocated' is removed now at the end, but still set to mark reserved slots. There was another error handling difference that I found and fixed - it's not a problem for unused variables to have wrong regset in their reservations.

[Francisco Casas (＠fcasas)]

Francisco Casas (@fcasas) commented about libs/vkd3d-shader/hlsl_codegen.c:
> -
> - LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
> - {
> -
>    enum hlsl_regset regset;
> -
> -
>    if (!hlsl_type_is_resource(var->data_type))
> -
>        continue;
> -
>    if (!var->reg_reservation.reg_type)
> -
>        continue;
> -
>    regset = hlsl_type_get_regset(var->data_type);
> -
>    if (var->regs[regset].allocated && !var->last_read)
> -
>    {
> -
>        var->regs[regset].allocated = false;
>    }
> }
>
> }

I am thinking on remove hlsl_type_get_regset(.) in future patches because it is not defined for structs (would probably reach unreacheable code here).

I think it is better to do this at the end of allocate_objects(.), and just for the regset that is passed as argument.