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

[Nikolay Sivov (＠nsivov)]

--
  v3: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.
 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150

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

[Nikolay Sivov]

From: Nikolay Sivov <nsivov(a)codeweavers.com>

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

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 54f3292b..7046404f 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -1291,13 +1291,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;
 }
@@ -1456,7 +1456,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 3e631dd9..271b1c84 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -1072,7 +1072,7 @@ struct hlsl_ir_store *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hl
 struct hlsl_ir_store *hlsl_new_store_component(struct hlsl_ctx *ctx, struct hlsl_block *block,
         const struct hlsl_deref *lhs, unsigned int comp, struct hlsl_ir_node *rhs);
 
-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 3873a939..f275ccbf 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -436,12 +436,59 @@ 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.");
+        return NULL;
+    }
+
+    /* 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_fixme(ctx, loc, "Unroll attribute with iteration count.");
+                return NULL;
+            }
+        }
+        else
+        {
+            if (!strcmp(attr->name, "loop")
+                    || !strcmp(attr->name, "fastopt")
+                    || !strcmp(attr->name, "allow_uav_condition"))
+            {
+                hlsl_fixme(ctx, loc, "Unhandled attribute %s.", attr->name);
+                return NULL;
+            }
+        }
+    }
 
     if (!(list = make_empty_list(ctx)))
         goto oom;
@@ -4033,6 +4080,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
 
@@ -4327,6 +4375,14 @@ attribute_list:
             $$.attrs[$$.count++] = $2;
         }
 
+attribute_list_optional:
+      %empty
+        {
+            $$.count = 0;
+            $$.attrs = NULL;
+        }
+      | attribute_list
+
 func_declaration:
       func_prototype compound_statement
         {
@@ -5294,22 +5350,26 @@ 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);
+            if (!($$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@1)))
+                YYABORT;
         }
-    | 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);
+            if (!($$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@1)))
+                YYABORT;
         }
-    | 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);
+            if (!($$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1)))
+                YYABORT;
             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);
+            if (!($$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1)))
+                YYABORT;
             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);
+}
-- 
GitLab

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

[Matteo Bruni (＠Mystral)]

On Thu Apr  6 12:16:20 2023 +0000, Nikolay Sivov wrote:
> What I see is warnings for random attributes, not errors. Plain unroll
> means to avoid flow control instructions, unroll(X) determines a number
> of iterations to unroll, and could specify lower number than loop
> normally would run for.
> What's not obvious here is that e.g. unroll(3) means that the loop is (fully, not partially) unrolled exactly 3 times, which changes semantics if the loop would have run for more than 3 iterations. It's absolutely crazy but that's what native does...

Maybe we could just FIXME() for the "unroll" (no count) case? Or WARN()? Just so that we get some trace reminding us that we aren't doing the correct thing, even though it shouldn't matter most of the time.

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