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

[Nikolay Sivov (＠nsivov)]

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

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

[Nikolay Sivov]

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

---
 libs/vkd3d-shader/hlsl.y   | 77 ++++++++++++++++++++++++++++++++------
 tests/hlsl-for.shader_test |  2 +-
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 79ec970a..8cc3c14a 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, 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;
@@ -4032,7 +4079,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 attribute_list_optional
 
 %type <boolval> boolean
 
@@ -4327,6 +4374,14 @@ attribute_list:
             $$.attrs[$$.count++] = $2;
         }
 
+attribute_list_optional:
+      %empty
+        {
+            $$.count = 0;
+            $$.attrs = NULL;
+        }
+      | attribute_list
+
 func_declaration:
       func_prototype compound_statement
         {
@@ -5294,22 +5349,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..758f1e04 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)
-- 
GitLab

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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> +    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, struct vkd3d_shader_location loc)
> Since you're already rewriting a good chunk of `create_loop()` and all its call sites, please convert the last parameter to `const struct vkd3d_shader_location *loc`. Also, I think `create_loop()` is responsible for freeing `attributes`.

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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> %type <attr> attribute
>
> -%type <attr_list> attribute_list
> +%type <attr_list> attribute_list attribute_list_optional

Minor, but we put only one rule name per `%type` directive.

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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> +    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, 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;
> +    }
> Please add a test that triggers this. The only `for` test we currently have fails for other reasons (`break` and `continue` are unsupported), so it's not a good indicator here, it's better to create another one that otherwise succeeds (both for the single and duplicated attribute). That will trigger a segmentation fault, which should be fixed. I believe the segmentation fault is caused by `create_loop()` returning `NULL` instead of an empty list on errors, which means that it's not (entirely) your commit's fault, but please fix it anyway.

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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> +            {
> +                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;
> +            }
> +        }
> +    }
> Ignoring invalid attributes doesn't seem the right thing to do. The native compiler fails on invalid attributes, from my tests. Also, why are you triggering a `fixme` on `unroll(X)` and not on plain `unroll`?

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

[Nikolay Sivov (＠nsivov)]

On Thu Apr  6 11:25:59 2023 +0000, Giovanni Mascellani wrote:
> Minor, but we put only one rule name per `%type` directive.
> There is another instance when there are several, but I don't see how this is important.

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

[Nikolay Sivov (＠nsivov)]

On Thu Apr  6 11:26:00 2023 +0000, Giovanni Mascellani wrote:
> Ignoring invalid attributes doesn't seem the right thing to do. The
> native compiler fails on invalid attributes, from my tests. Also, why
> are you triggering a `fixme` on `unroll(X)` and not on plain `unroll`?
> 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.

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