[PATCH 0/1] MR655: vkd3d-shader/hlsl: Emit fixme on non-direct resource stores.

[Francisco Casas (＠fcasas)]

Co-authored-by: Giovanni Mascellani

These may happen when storing to structured buffers, and we are not
handling them properly yet. The included test reaches unreacheable code
before this patch.

Storing to buffers is complicated since we need to split the index
chain in two paths:
- The path within the variable where the resource is.
- The subpath to the part of the resource element that is being stored
  to.

For now, we will emit a fixme when the index chain in the lhs is not a
direct resource access.

[PATCH 1/1] vkd3d-shader/hlsl: Emit fixme on non-direct resource stores.

[Francisco Casas]

From: Francisco Casas

Co-authored-by: Giovanni Mascellani

These may happen when storing to structured buffers, and we are not
handling them properly yet. The included test reaches unreacheable code
before this patch.

Storing to buffers is complicated since we need to split the index
chain in two paths:
- The path within the variable where the resource is.
- The subpath to the part of the resource element that is being stored
  to.

For now, we will emit a fixme when the index chain in the lhs is not a
direct resource access.
---
 libs/vkd3d-shader/hlsl.c                      |  9 +++++++++
 libs/vkd3d-shader/hlsl.h                      |  1 +
 libs/vkd3d-shader/hlsl.y                      |  8 +++++++-
 tests/hlsl/uav-rwstructuredbuffer.shader_test | 15 +++++++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 3d068ac6d..7286c5ca0 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -1554,6 +1554,15 @@ bool hlsl_index_is_resource_access(struct hlsl_ir_index *index)
     return index->val.node->data_type->class == HLSL_CLASS_OBJECT;
 }

+bool hlsl_index_chain_has_resource_access(struct hlsl_ir_index *index)
+{
+    if (hlsl_index_is_resource_access(index))
+        return true;
+    if (index->val.node->type == HLSL_IR_INDEX)
+        return hlsl_index_chain_has_resource_access(hlsl_ir_index(index->val.node));
+    return false;
+}
+
 struct hlsl_ir_node *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val,
         struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc)
 {
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 43bc079db..392d4cb58 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -1252,6 +1252,7 @@ bool hlsl_new_store_component(struct hlsl_ctx *ctx, struct hlsl_block *block,

bool hlsl_index_is_noncontiguous(struct hlsl_ir_index *index);
 bool hlsl_index_is_resource_access(struct hlsl_ir_index *index);
+bool hlsl_index_chain_has_resource_access(struct hlsl_ir_index *index);

struct hlsl_ir_node *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val,
         struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 37a372893..7cdb07f97 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1915,7 +1915,7 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct hlsl_blo
         }
     }

-    if (lhs->type == HLSL_IR_INDEX && hlsl_index_is_resource_access(hlsl_ir_index(lhs)))
+    if (lhs->type == HLSL_IR_INDEX && hlsl_index_chain_has_resource_access(hlsl_ir_index(lhs)))
     {
         struct hlsl_ir_node *coords = hlsl_ir_index(lhs)->idx.node;
         struct hlsl_deref resource_deref;
@@ -1923,6 +1923,12 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct hlsl_blo
         struct hlsl_ir_node *store;
         unsigned int dim_count;

+        if (!hlsl_index_is_resource_access(hlsl_ir_index(lhs)))
+        {
+            hlsl_fixme(ctx, &lhs->loc, "Non-direct structured resource store.");
+            return NULL;
+        }
+
         if (!hlsl_init_deref_from_index_chain(ctx, &resource_deref, hlsl_ir_index(lhs)->val.node))
             return NULL;

diff --git a/tests/hlsl/uav-rwstructuredbuffer.shader_test b/tests/hlsl/uav-rwstructuredbuffer.shader_test
index 37f52ee75..f962e18e0 100644
--- a/tests/hlsl/uav-rwstructuredbuffer.shader_test
+++ b/tests/hlsl/uav-rwstructuredbuffer.shader_test
@@ -108,3 +108,18 @@ float4 main() : sv_target1
 {
     return 0;
 }
+
+
+[pixel shader todo]
+struct apple
+{
+    float3 a, x;
+};
+
+RWStructuredBuffer<apple> u;
+
+float4 main() : sv_target
+{
+    u[0].x = float3(30.0, 40.0, 50.0);
+    return 0;
+}

[Zebediah Figura (＠zfigura)]

Accepting because this is not a big thing and we do want this bug fixed, but: in general I wouldn't put helpers like hlsl_index_chain_has_resource_access() in hlsl.c unless it's actually necessary to have them there. Yeah, I know that the divide is kind of vague, and we do keep all of the hlsl_new_*() stuff in hlsl.c regardless, but this is pretty much a helper that's not only currently only used by parsing code, I don't think it'll ever be used by anything else.

I also have this nagging feeling that there's a better way to structure this function, mainly because hlsl_index_chain_has_resource_access() unwraps something which is then subsequently unwrapped again. I feel like there's probably a way to return the interior node from hlsl_init_deref_from_index_chain(), factor out that function call from the individual branches. But maybe it's not worth doing, and I guess it also depends how we're going to represent structured buffer stores exactly.

[PATCH 0/1] MR655: vkd3d-shader/hlsl: Emit fixme on non-direct resource stores. - approved

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.

[PATCH 0/1] MR655: vkd3d-shader/hlsl: Emit fixme on non-direct resource stores. - approved

[Giovanni Mascellani (＠giomasce)]

This merge request was approved by Giovanni Mascellani.

[PATCH 0/1] MR655: vkd3d-shader/hlsl: Emit fixme on non-direct resource stores. - approved

[Henri Verbeet (＠hverbeet)]

This merge request was approved by Henri Verbeet.