[PATCH v6 0/1] MR150: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.

[Nikolay Sivov (＠nsivov)]

--
  v6: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.

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

[PATCH v6 1/1] vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.

[Nikolay Sivov]

From: Nikolay Sivov

---
 libs/vkd3d-shader/hlsl.c   |  6 ++--
 libs/vkd3d-shader/hlsl.h   |  2 +-
 libs/vkd3d-shader/hlsl.y   | 70 ++++++++++++++++++++++++++++++++------
 tests/hlsl-for.shader_test | 26 +++++++++++++-
 4 files changed, 89 insertions(+), 15 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 869638a6..d87be198 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -1392,13 +1392,13 @@ struct hlsl_ir_jump *hlsl_new_jump(struct hlsl_ctx *ctx, enum hlsl_ir_jump_type
     return jump;
 }

-struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_location loc)
+struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc)
 {
     struct hlsl_ir_loop *loop;

if (!(loop = hlsl_alloc(ctx, sizeof(*loop))))
         return NULL;
-    init_node(&loop->node, HLSL_IR_LOOP, NULL, &loc);
+    init_node(&loop->node, HLSL_IR_LOOP, NULL, loc);
     hlsl_block_init(&loop->body);
     return loop;
 }
@@ -1557,7 +1557,7 @@ static struct hlsl_ir_node *clone_loop(struct hlsl_ctx *ctx, struct clone_instr_
 {
     struct hlsl_ir_loop *dst;

-    if (!(dst = hlsl_new_loop(ctx, src->node.loc)))
+    if (!(dst = hlsl_new_loop(ctx, &src->node.loc)))
         return NULL;
     if (!clone_block(ctx, &dst->body, &src->body, map))
     {
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index b38fba1c..5da78b3e 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -1090,7 +1090,7 @@ bool hlsl_index_is_noncontiguous(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);
-struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_location loc);
+struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc);
 struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx,
         const struct hlsl_resource_load_params *params, const struct vkd3d_shader_location *loc);
 struct hlsl_ir_resource_store *hlsl_new_resource_store(struct hlsl_ctx *ctx, const struct hlsl_deref *resource,
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 0ddae6ee..ed72f817 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -436,12 +436,53 @@ enum loop_type
     LOOP_DO_WHILE
 };

-static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, struct list *init, struct list *cond,
-        struct list *iter, struct list *body, struct vkd3d_shader_location loc)
+static bool attribute_list_has_duplicates(const struct parse_attribute_list *attrs)
+{
+    unsigned int i, j;
+
+    for (i = 0; i < attrs->count; ++i)
+    {
+        for (j = i + 1; j < attrs->count; ++j)
+        {
+            if (!strcmp(attrs->attrs[i]->name, attrs->attrs[j]->name))
+                 return true;
+        }
+    }
+
+    return false;
+}
+
+static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const struct parse_attribute_list *attributes,
+        struct list *init, struct list *cond, struct list *iter, struct list *body, const struct vkd3d_shader_location *loc)
 {
     struct list *list = NULL;
     struct hlsl_ir_loop *loop = NULL;
     struct hlsl_ir_if *cond_jump = NULL;
+    unsigned int i;
+
+    if (attribute_list_has_duplicates(attributes))
+        hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Found duplicate attribute.");
+
+    /* Ignore unroll(0) attribute, and any invalid attribute. */
+    for (i = 0; i < attributes->count; ++i)
+    {
+        const struct hlsl_attribute *attr = attributes->attrs[i];
+        if (!strcmp(attr->name, "unroll"))
+        {
+            if (attr->args_count)
+            {
+                hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unroll attribute with iteration count.");
+            }
+            else
+            {
+                FIXME("Loop unrolling is not implemented.\n");
+            }
+        }
+        else
+        {
+            hlsl_warning(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unrecognized attribute %s.", attr->name);
+        }
+    }

if (!(list = make_empty_list(ctx)))
         goto oom;
@@ -4024,6 +4065,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type
 %type <attr> attribute

%type <attr_list> attribute_list
+%type <attr_list> attribute_list_optional

%type <boolval> boolean

@@ -4318,6 +4360,14 @@ attribute_list:
             $$.attrs[$$.count++] = $2;
         }

+attribute_list_optional:
+      %empty
+        {
+            $$.count = 0;
+            $$.attrs = NULL;
+        }
+      | attribute_list
+
 func_declaration:
       func_prototype compound_statement
         {
@@ -5285,22 +5335,22 @@ if_body:
         }

loop_statement:
-      KW_WHILE '(' expr ')' statement
+      attribute_list_optional KW_WHILE '(' expr ')' statement
         {
-            $$ = create_loop(ctx, LOOP_WHILE, NULL, $3, NULL, $5, @1);
+            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@1);
         }
-    | KW_DO statement KW_WHILE '(' expr ')' ';'
+    | attribute_list_optional KW_DO statement KW_WHILE '(' expr ')' ';'
         {
-            $$ = create_loop(ctx, LOOP_DO_WHILE, NULL, $5, NULL, $2, @1);
+            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@1);
         }
-    | KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
+    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, $4, $5, $6, $8, @1);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1);
             hlsl_pop_scope(ctx);
         }
-    | KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement
+    | attribute_list_optional KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, $4, $5, $6, $8, @1);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1);
             hlsl_pop_scope(ctx);
         }

diff --git a/tests/hlsl-for.shader_test b/tests/hlsl-for.shader_test
index ae7146c9..e0fdaa47 100644
--- a/tests/hlsl-for.shader_test
+++ b/tests/hlsl-for.shader_test
@@ -9,7 +9,7 @@ float4 main(float tex : texcoord) : sv_target
 {
     int i;
     float x = 0.0;
-    for (i = 0; i < 10; i++)
+    [unroll] for (i = 0; i < 10; i++)
     {
         x += i;
         if (tex > 0.5 && i == 5)
@@ -26,3 +26,27 @@ todo draw quad
 probe (  0, 0, 159, 480) rgba (10.0, 35.0, 0.0, 0.0)
 probe (161, 0, 479, 480) rgba (10.0, 38.0, 0.0, 0.0)
 probe (481, 0, 640, 480) rgba ( 5.0, 10.0, 0.0, 0.0)
+
+[pixel shader]
+float4 main(float tex : texcoord) : sv_target
+{
+    int i;
+    float x = 0.0;
+    [unroll] [attr1] for (i = 0; i < 10; i++)
+    {
+        x += i;
+    }
+    return float4(i, x, 0.0, 0.0);
+}
+
+[pixel shader fail]
+float4 main(float tex : texcoord) : sv_target
+{
+    int i;
+    float x = 0.0;
+    [unroll] [unroll] for (i = 0; i < 10; i++)
+    {
+        x += i;
+    }
+    return float4(i, x, 0.0, 0.0);
+}

[Zebediah Figura (＠zfigura)]

> on other attributes, we should `hlsl_fixme()`, because (AFAIU) there is at least an attribute that could change the semantics if ignored (e.g., `allow_uav_condition`). We could `hlsl_warning()` for the attributes we already consider relatively safe to ignore.

Yes, I agree with this part. v4 had it as hlsl_fixme().

In general I think we should put in the effort to avoid ever emitting invalid bytecode.