[PATCH v4 0/3] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.

[Nikolay Sivov (＠nsivov)]

Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>

--
  v4: vkd3d-shader/tpf: Write 'continue' instruction.
      vkd3d-shader/hlsl: Parse 'continue' statement.
      vkd3d-shader/hlsl: Allow 'break' instructions in loops.
 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245

[PATCH v4 1/3] vkd3d-shader/hlsl: Allow 'break' instructions in loops.

[Nikolay Sivov]

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

---
 libs/vkd3d-shader/hlsl.h   |  2 ++
 libs/vkd3d-shader/hlsl.y   | 52 +++++++++++++++++++++++++++++++-------
 tests/hlsl/for.shader_test |  7 +++++
 3 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index 28c7c6914..eceead8cb 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -682,6 +682,8 @@ struct hlsl_scope
     struct rb_tree types;
     /* Scope containing this scope. This value is NULL for the global scope. */
     struct hlsl_scope *upper;
+    /* The scope was created for the loop statement. */
+    bool loop;
 };
 
 struct hlsl_profile_info
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 60d6514c9..36face242 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -4308,6 +4308,14 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type
     }
 }
 
+static struct hlsl_scope *get_loop_scope(struct hlsl_scope *scope)
+{
+    if (scope->loop)
+        return scope;
+
+    return scope->upper ? get_loop_scope(scope->upper) : NULL;
+}
+
 }
 
 %locations
@@ -5013,6 +5021,13 @@ scope_start:
             hlsl_push_scope(ctx);
         }
 
+loop_scope_start:
+      %empty
+        {
+            hlsl_push_scope(ctx);
+            ctx->cur_scope->loop = true;
+        }
+
 var_identifier:
       VAR_IDENTIFIER
     | NEW_IDENTIFIER
@@ -5778,7 +5793,24 @@ statement:
     | loop_statement
 
 jump_statement:
-      KW_RETURN expr ';'
+      KW_BREAK ';'
+        {
+            struct hlsl_ir_node *jump;
+
+            /* TODO: allow 'break' in the 'switch' statements. */
+
+            if (!get_loop_scope(ctx->cur_scope))
+            {
+                hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Incorrect context for a 'break' statement.");
+            }
+
+            if (!($$ = make_empty_list(ctx)))
+                YYABORT;
+            if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_BREAK, NULL, &@1)))
+                YYABORT;
+            list_add_tail($$, &jump->entry);
+        }
+    | KW_RETURN expr ';'
         {
             if (!add_return(ctx, $2, node_from_list($2), &@1))
                 YYABORT;
@@ -5847,22 +5879,24 @@ if_body:
         }
 
 loop_statement:
-      attribute_list_optional KW_WHILE '(' expr ')' statement
+      attribute_list_optional loop_scope_start KW_WHILE '(' expr ')' statement
         {
-            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@2);
+            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $5, NULL, $7, &@3);
+            hlsl_pop_scope(ctx);
         }
-    | attribute_list_optional KW_DO statement KW_WHILE '(' expr ')' ';'
+    | attribute_list_optional loop_scope_start KW_DO statement KW_WHILE '(' expr ')' ';'
         {
-            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@2);
+            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
+            hlsl_pop_scope(ctx);
         }
-    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
+    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
             hlsl_pop_scope(ctx);
         }
-    | attribute_list_optional KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement
+    | attribute_list_optional loop_scope_start KW_FOR '(' declaration expr_statement expr_optional ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
             hlsl_pop_scope(ctx);
         }
 
diff --git a/tests/hlsl/for.shader_test b/tests/hlsl/for.shader_test
index b68969819..beb521e77 100644
--- a/tests/hlsl/for.shader_test
+++ b/tests/hlsl/for.shader_test
@@ -54,3 +54,10 @@ float4 main(float tex : texcoord) : sv_target
     }
     return float4(i, x, 0.0, 0.0);
 }
+
+[pixel shader fail]
+float4 main() : sv_target
+{
+    break;
+    return float4(0.0, 0.0, 0.0, 0.0);
+}
-- 
GitLab


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

[PATCH v4 2/3] vkd3d-shader/hlsl: Parse 'continue' statement.

[Nikolay Sivov]

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

---
 libs/vkd3d-shader/hlsl.c |  2 ++
 libs/vkd3d-shader/hlsl.h |  2 ++
 libs/vkd3d-shader/hlsl.y | 57 +++++++++++++++++++++++++++++++---------
 3 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
index 3a1e17797..4648cc77c 100644
--- a/libs/vkd3d-shader/hlsl.c
+++ b/libs/vkd3d-shader/hlsl.c
@@ -1842,6 +1842,7 @@ static struct hlsl_scope *hlsl_new_scope(struct hlsl_ctx *ctx, struct hlsl_scope
         return NULL;
     list_init(&scope->vars);
     rb_init(&scope->types, compare_hlsl_types_rb);
+    hlsl_block_init(&scope->loop_iterator);
     scope->upper = upper;
     list_add_tail(&ctx->scopes, &scope->entry);
     return scope;
@@ -3281,6 +3282,7 @@ static void hlsl_ctx_cleanup(struct hlsl_ctx *ctx)
         LIST_FOR_EACH_ENTRY_SAFE(var, next_var, &scope->vars, struct hlsl_ir_var, scope_entry)
             hlsl_free_var(var);
         rb_destroy(&scope->types, NULL, NULL);
+        hlsl_block_cleanup(&scope->loop_iterator);
         vkd3d_free(scope);
     }
 
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
index eceead8cb..f83f06d96 100644
--- a/libs/vkd3d-shader/hlsl.h
+++ b/libs/vkd3d-shader/hlsl.h
@@ -684,6 +684,8 @@ struct hlsl_scope
     struct hlsl_scope *upper;
     /* The scope was created for the loop statement. */
     bool loop;
+    /* Block of iterator instructions of a 'for' loop. */
+    struct hlsl_block loop_iterator;
 };
 
 struct hlsl_profile_info
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 36face242..b24c87544 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -482,10 +482,10 @@ static bool attribute_list_has_duplicates(const struct parse_attribute_list *att
     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)
+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 *body, const struct vkd3d_shader_location *loc)
 {
-    struct hlsl_block body_block;
+    struct hlsl_block body_block, iter_block;
     struct hlsl_ir_node *loop;
     unsigned int i;
 
@@ -532,8 +532,9 @@ static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const
 
     list_move_tail(&body_block.instrs, body);
 
-    if (iter)
-        list_move_tail(&body_block.instrs, iter);
+    if (!hlsl_clone_block(ctx, &iter_block, &ctx->cur_scope->loop_iterator))
+        goto oom;
+    hlsl_block_add_block(&body_block, &iter_block);
 
     if (type == LOOP_DO_WHILE)
         list_move_tail(&body_block.instrs, cond);
@@ -549,7 +550,6 @@ static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const
 oom:
     destroy_instr_list(init);
     destroy_instr_list(cond);
-    destroy_instr_list(iter);
     destroy_instr_list(body);
     return NULL;
 }
@@ -4480,6 +4480,7 @@ static struct hlsl_scope *get_loop_scope(struct hlsl_scope *scope)
 %type <list> jump_statement
 %type <list> logicand_expr
 %type <list> logicor_expr
+%type <list> loop_iterator
 %type <list> loop_statement
 %type <list> mul_expr
 %type <list> postfix_expr
@@ -5810,6 +5811,27 @@ jump_statement:
                 YYABORT;
             list_add_tail($$, &jump->entry);
         }
+    | KW_CONTINUE ';'
+        {
+            struct hlsl_ir_node *jump;
+            struct hlsl_scope *scope;
+            struct hlsl_block iter;
+
+            if (!(scope = get_loop_scope(ctx->cur_scope)))
+            {
+                hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Incorrect context for a 'continue' statement.");
+            }
+
+            if (!($$ = make_empty_list(ctx)))
+                YYABORT;
+
+            if (!hlsl_clone_block(ctx, &iter, &scope->loop_iterator))
+                YYABORT;
+            if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_CONTINUE, NULL, &@1)))
+                YYABORT;
+            list_move_tail($$, &iter.instrs);
+            list_add_tail($$, &jump->entry);
+        }
     | KW_RETURN expr ';'
         {
             if (!add_return(ctx, $2, node_from_list($2), &@1))
@@ -5878,25 +5900,36 @@ if_body:
             $$.else_block = list_to_block($3);
         }
 
+loop_iterator:
+      expr_optional
+        {
+            struct hlsl_scope *scope = ctx->cur_scope;
+
+            if ($1)
+            {
+                list_move_tail(&scope->loop_iterator.instrs, $1);
+            }
+        }
+
 loop_statement:
       attribute_list_optional loop_scope_start KW_WHILE '(' expr ')' statement
         {
-            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $5, NULL, $7, &@3);
+            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $5, $7, &@3);
             hlsl_pop_scope(ctx);
         }
     | attribute_list_optional loop_scope_start KW_DO statement KW_WHILE '(' expr ')' ';'
         {
-            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
+            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, $4, &@3);
             hlsl_pop_scope(ctx);
         }
-    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
+    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement loop_iterator ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $9, &@3);
             hlsl_pop_scope(ctx);
         }
-    | attribute_list_optional loop_scope_start KW_FOR '(' declaration expr_statement expr_optional ')' statement
+    | attribute_list_optional loop_scope_start KW_FOR '(' declaration expr_statement loop_iterator ')' statement
         {
-            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
+            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $9, &@3);
             hlsl_pop_scope(ctx);
         }
 
-- 
GitLab


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

[PATCH v4 3/3] vkd3d-shader/tpf: Write 'continue' instruction.

[Nikolay Sivov]

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

---
 libs/vkd3d-shader/tpf.c    | 4 ++++
 tests/hlsl/for.shader_test | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c
index 677243e15..fa1d0c207 100644
--- a/libs/vkd3d-shader/tpf.c
+++ b/libs/vkd3d-shader/tpf.c
@@ -4781,6 +4781,10 @@ static void write_sm4_jump(struct hlsl_ctx *ctx,
             instr.opcode = VKD3D_SM4_OP_BREAK;
             break;
 
+        case HLSL_IR_JUMP_CONTINUE:
+            instr.opcode = VKD3D_SM4_OP_CONTINUE;
+            break;
+
         case HLSL_IR_JUMP_DISCARD_NZ:
         {
             instr.opcode = VKD3D_SM4_OP_DISCARD | VKD3D_SM4_CONDITIONAL_NZ;
diff --git a/tests/hlsl/for.shader_test b/tests/hlsl/for.shader_test
index beb521e77..e0b0edc11 100644
--- a/tests/hlsl/for.shader_test
+++ b/tests/hlsl/for.shader_test
@@ -4,7 +4,7 @@ void main(out float tex : texcoord, inout float4 pos : sv_position)
     tex = pos.x;
 }
 
-[pixel shader todo]
+[pixel shader]
 float4 main(float tex : texcoord) : sv_target
 {
     int i;
@@ -22,7 +22,7 @@ float4 main(float tex : texcoord) : sv_target
 }
 
 [test]
-todo draw quad
+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)
-- 
GitLab

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

[Nikolay Sivov (＠nsivov)]

On Wed Jun 28 17:17:41 2023 +0000, Zebediah Figura wrote:
> I was initially opposed to storing the context like that, but
> reconsidering, I guess I don't have a good reason to be opposed to it.
> Now pushed with iterator block stored in a loop scope.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37252

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> -            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@2);
> +            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $5, NULL, $7, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_DO statement KW_WHILE '(' expr ')' ';'
> +    | attribute_list_optional loop_scope_start KW_DO statement KW_WHILE '(' expr ')' ';'
>          {
> -            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@2);
> +            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
> +            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
> You need to fix the indices for the other arguments.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37285

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> -            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@2);
> +            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
> +            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
>              hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' declaration expr_statement expr_optional ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
> +            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
> Here too.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37286

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> +            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
> +            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
>              hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' declaration expr_statement expr_optional ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2);
> +            $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@3);
>              hlsl_pop_scope(ctx);
> There is some problem with how loop scoping is handled here. For example, this is accepted by native and not by us:
float4 main() : sv_target
{
    for (int y = 0; y < 1000; y++);
    return float4(y, 0.0, 0.0, 0.0);
}

However this is broken also before this MR, so I'm not insisting on this, just wanted to mention it. I'm not sure right now on what is the best solution to handle scopes here, and the  proposal doesn't look more broken than the current state, so it's ok for me.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37287

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> | loop_statement
>
> jump_statement:
> -      KW_RETURN expr ';'
> +      KW_BREAK ';'
> +        {
> +            struct hlsl_ir_node *jump;
> +
> +            /* TODO: allow 'break' in the 'switch' statements. */

That shouldn't be too hard to at least detect, and emit an `hlsl_fixme()` in case.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37288

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
> $$.else_block = list_to_block($3);
>          }
>
> +loop_iterator:
> +      expr_optional
> +        {
> +            struct hlsl_scope *scope = ctx->cur_scope;
> +
> +            if ($1)
> +            {
> +                list_move_tail(&scope->loop_iterator.instrs, $1);
> +            }
> +        }

I am not necessarily opposed to that, but wouldn't it be simpler to do this in `create_loop()`, without making the parser more complicated? I think that would be easier to read.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37289

[Giovanni Mascellani (＠giomasce)]

The new proposal looks mostly good to me, except for a few suggestions.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37290

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 10:01:38 2023 +0000, Giovanni Mascellani wrote:
> You need to fix the indices for the other arguments.
> Which ones need adjusting? I think it fine, only KW_FOR index moves.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37294

[Giovanni Mascellani (＠giomasce)]

On Thu Jun 29 10:27:48 2023 +0000, Nikolay Sivov wrote:
> Which ones need adjusting? I think it fine, only KW_FOR index moves.
> Oh, yes, I had misread. Sorry.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37295

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 10:01:41 2023 +0000, Giovanni Mascellani wrote:
> I am not necessarily opposed to that, but wouldn't it be simpler to do
> this in `create_loop()`, without making the parser more complicated? I
> think that would be easier to read.
> I don't quite get what you are proposing. Loop body is already parsed by the time we get to create_loop().

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37296

[Giovanni Mascellani (＠giomasce)]

On Thu Jun 29 10:33:17 2023 +0000, Nikolay Sivov wrote:
> I don't quite get what you are proposing. Loop body is already parsed by
> the time we get to create_loop().
> Ah yes, that's right too.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37298

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 10:01:40 2023 +0000, Giovanni Mascellani wrote:
> That shouldn't be too hard to at least detect, and emit an
> `hlsl_fixme()` in case.
> We don't handle switches yet.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37299

[PATCH v4 0/3] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops. - approved

[Giovanni Mascellani (＠giomasce)]

This merge request was approved by Giovanni Mascellani.
-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 10:46:21 2023 +0000, Giovanni Mascellani wrote:
> There is some problem with how loop scoping is handled here. For
> example, this is accepted by native and not by us:
> ```
> float4 main() : sv_target
> {
>     for (int y = 0; y < 1000; y++);
>     return float4(y, 0.0, 0.0, 0.0);
> }
> ```
> However this is broken also before this MR, so I'm not insisting on
> this, just wanted to mention it. I'm not sure right now on what is the
> best solution to handle scopes here, and the  proposal doesn't look more
> broken than the current state, so it's ok for me.
> Yes, this is something unrelated, and unexpected too. Essentially what it does is replaces any declarations in outer scope, but only one level up. If you had 'int y = 1;' in your example before the loop, compiler produces a warning, but still uses what was declared in the loop. If however you enclose the loop in its own block, it does not do that, and loop will be eliminated.

Definitely something to fix, and trivial to test too.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37302

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
> $$.else_block = list_to_block($3);
>          }
>
> +loop_iterator:
> +      expr_optional
> +        {
> +            struct hlsl_scope *scope = ctx->cur_scope;
> +
> +            if ($1)
> +            {
> +                list_move_tail(&scope->loop_iterator.instrs, $1);
> +            }
> +        }
> +

After seeing it in action, this really doesn't feel like the simplest solution, compared to some of the others that have been proposed.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37345

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
> | loop_statement
>
> jump_statement:
> -      KW_RETURN expr ';'
> +      KW_BREAK ';'
> +        {
> +            struct hlsl_ir_node *jump;
> +
> +            /* TODO: allow 'break' in the 'switch' statements. */
> +
> +            if (!get_loop_scope(ctx->cur_scope))
> +            {
> +                hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Incorrect context for a 'break' statement.");

I feel like we could be more specific without losing anything, like, "'break' must be inside of a loop."

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37346

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
> -      attribute_list_optional KW_WHILE '(' expr ')' statement
> +      attribute_list_optional loop_scope_start KW_WHILE '(' expr ')' statement
>          {
> -            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@2);
> +            $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $5, NULL, $7, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_DO statement KW_WHILE '(' expr ')' ';'
> +    | attribute_list_optional loop_scope_start KW_DO statement KW_WHILE '(' expr ')' ';'
>          {
> -            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@2);
> +            $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $7, NULL, $4, &@3);
> +            hlsl_pop_scope(ctx);
>          }
> -    | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement
> +    | attribute_list_optional loop_scope_start KW_FOR '(' expr_statement expr_statement expr_optional ')' statement
> Why do we need to move the token?

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37347

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 16:55:44 2023 +0000, Zebediah Figura wrote:
> Why do we need to move the token?
> We don't need to, I did that so all types of loops look the same. It could go after loop keyword.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37358

[Nikolay Sivov (＠nsivov)]

On Thu Jun 29 16:55:42 2023 +0000, Zebediah Figura wrote:
> After seeing it in action, this really doesn't feel like the simplest
> solution, compared to some of the others that have been proposed.
> It's spread out more, that's true, but it is simpler than trying to fix up instructions later, is easier to understand and shorter to read.

Regarding https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37287, we should probably reuse existing scope for initialization part.

So, any suggestion how to proceed? I find an option of always creating correct list for KW_CONTINUE better.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37365

[Zebediah Figura (＠zfigura)]

On Thu Jun 29 17:48:02 2023 +0000, Nikolay Sivov wrote:
> We don't need to, I did that so all types of loops look the same. It
> could go after loop keyword.
> It's a fair enough reason, I just wondered if there was something more concrete to it.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37370

[Giovanni Mascellani (＠giomasce)]

On Thu Jun 29 17:54:53 2023 +0000, Nikolay Sivov wrote:
> It's spread out more, that's true, but it is simpler than trying to fix
> up instructions later, is easier to understand and shorter to read.
> Regarding
> https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37287, we
> should probably reuse existing scope for initialization part.
> So, any suggestion how to proceed? I find an option of always creating
> correct list for KW_CONTINUE better.
> Generally I prefer the new approach (store the iteration instruction list and then copy it directly before each `continue`) rather than the old one (first generate code without the iteration instructions and then patch it). One of the reasons is that, as you said, it's good that the IR has a well-defined meaning independently of the processing stage.

I can't exclude that some details could be refined a little bit, but the current proposal is fine for me.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37377

[Zebediah Figura (＠zfigura)]

On Thu Jun 29 18:26:09 2023 +0000, Giovanni Mascellani wrote:
> Generally I prefer the new approach (store the iteration instruction
> list and then copy it directly before each `continue`) rather than the
> old one (first generate code without the iteration instructions and then
> patch it). One of the reasons is that, as you said, it's good that the
> IR has a well-defined meaning independently of the processing stage.
> I can't exclude that some details could be refined a little bit, but the
> current proposal is fine for me.
> I don't know if I can describe why in any exact terms, but it doesn't feel as easy to understand to me.
> Generally I prefer the new approach (store the iteration instruction list and then copy it directly before each `continue`) rather than the old one (first generate code without the iteration instructions and then patch it). One of the reasons is that, as you said, it's good that the IR has a well-defined meaning independently of the processing stage.

I guess.

It might help if we were to arrange the rules more like

loop_statement:
      loop_header statement

loop_header:
      attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')'


and then loop_header can essentially call create_loop() as now but without filling the body yet, then loop_statement would take care of moving the "statement" body into the loop instruction. That would also remove the need for "loop_scope_start".

Hmm, but this breaks with do-while...

---

Okay, so a minute later I realize that there's a problem which I think prevents this approach from working in the first place. 'continue' doesn't just need to apply the iterator, it needs to apply the conditional in the case of do-while loops, and we can't know what the conditional is until we've parsed the whole loop. [This is only a concern for do-while; for other loop types the conditional is executed at the head of the loop.] Example shader, sorry it's a bit complex:

```
uint m;

float4 main() : sv_target
{
    float4 f = 0;
    uint x = 0;

    do
    {
        if (x % 2 == 0)
            continue;
        f += 5;
    } while (x++ < m);

    return f;
}
```

compiled with sm4 you can see it increments x on every iteration.

So we either can fix up loops in create_loop(), or we can fix them up in a separate pass. In either case, for the sake of having a consistent meaning, it probably does make sense to have two separate "continue" jump types, though I'm not sure how to name them.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37676

[Giovanni Mascellani (＠giomasce)]

On Mon Jul  3 18:26:07 2023 +0000, Zebediah Figura wrote:
> I don't know if I can describe why in any exact terms, but it doesn't
> feel as easy to understand to me.
> > Generally I prefer the new approach (store the iteration instruction
> > list and then copy it directly before each `continue`) rather than the
> > old one (first generate code without the iteration instructions and then
> > patch it). One of the reasons is that, as you said, it's good that the
> > IR has a well-defined meaning independently of the processing stage.
> > I guess.
> > It might help if we were to arrange the rules more like
> > loop_statement:
> >       loop_header statement
> > loop_header:
> >       attribute_list_optional KW_FOR '(' scope_start expr_statement
> > expr_statement expr_optional ')'
> > and then loop_header can essentially call create_loop() as now but
> > without filling the body yet, then loop_statement would take care of
> > moving the "statement" body into the loop instruction. That would also
> > remove the need for "loop_scope_start".
> > Hmm, but this breaks with do-while...
>
> Okay, so a minute later I realize that there's a problem which I think
> prevents this approach from working in the first place. 'continue'
> doesn't just need to apply the iterator, it needs to apply the
> conditional in the case of do-while loops, and we can't know what the
> conditional is until we've parsed the whole loop. [This is only a
> concern for do-while; for other loop types the conditional is executed
> at the head of the loop.] Example shader, sorry it's a bit complex:
> ```
> uint m;
> float4 main() : sv_target
> {
>     float4 f = 0;
>     uint x = 0;
>     do
>     {
>         if (x % 2 == 0)
>             continue;
>         f += 5;
>     } while (x++ < m);
>     return f;
> }
> ```
> compiled with sm4 you can see it increments x on every iteration.
> So we either can fix up loops in create_loop(), or we can fix them up in
> a separate pass. In either case, for the sake of having a consistent
> meaning, it probably does make sense to have two separate "continue"
> jump types, though I'm not sure how to name them.
> Ugh, that's right. I hate that. I would name the new jump type something like `ITER_AND_CONTINUE`. The annoying detail is that that instruction has no meaning unless you know what the iteration block is, so it still happens that if you look at a IR dump is `ITER_AND_CONTINUE` you don't know which program is really being described. That instruction only make sense as long as you're inside the context in which the loop is being parsed, and at this point it doesn't look much better than just using `CONTINUE`, even if that's the solution that I originally disliked.

Unfortunately I don't have any better solution in mind for the "do while" loop. I can't exclude there might be some clever trick with bison to pre-parse the loop body, than parse the "while" condition and then parse the body for real, but even if there was I'm not sure it would be worth the additional complexity of the parser.

In conclusion, I think the best way to go is the original solution. I don't have a strong preference between overriding the `CONTINUE` jump, or introduce a new jump type.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37761

[Nikolay Sivov (＠nsivov)]

On Tue Jul  4 09:11:46 2023 +0000, Giovanni Mascellani wrote:
> Ugh, that's right. I hate that. I would name the new jump type something
> like `ITER_AND_CONTINUE`. The annoying detail is that that instruction
> has no meaning unless you know what the iteration block is, so it still
> happens that if you look at a IR dump is `ITER_AND_CONTINUE` you don't
> know which program is really being described. That instruction only make
> sense as long as you're inside the context in which the loop is being
> parsed, and at this point it doesn't look much better than just using
> `CONTINUE`, even if that's the solution that I originally disliked.
> Unfortunately I don't have any better solution in mind for the "do
> while" loop. I can't exclude there might be some clever trick with bison
> to pre-parse the loop body, than parse the "while" condition and then
> parse the body for real, but even if there was I'm not sure it would be
> worth the additional complexity of the parser.
> In conclusion, I think the best way to go is the original solution. I
> don't have a strong preference between overriding the `CONTINUE` jump,
> or introduce a new jump type.
> Why does it need a new jump type? I think what needs to happen is instruction block added before every 'continue', for any loop type - 'for' will use iteration expression, do-while(expr) and while(expr) will use 'expr'. In all three cases presumably, expression without side effects will be eventually removed. The way do-while() is handled is a bit different too, they seem to unroll first iteration, and turning the loop into a 'while' one, but we probably don't have to do that.

For while() loop generated structure is:

- evaluate condition, without storing side effects;
- break if false;
- body;
- evaluate condition, applying side effects this time;

do-while() turns into a while(), for() works the way this MR was going to make it work.
> In conclusion, I think the best way to go is the original solution. I don't have a strong preference between overriding the `CONTINUE` jump, or introduce a new jump type.

Original solution to retroactively add instructions by scanning loop bodies?

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37783

[Giovanni Mascellani (＠giomasce)]

> For while() loop generated structure is:
>
> * evaluate condition, without storing side effects;
> * break if false;
> * body;
> * evaluate condition, applying side effects this time;

I'm not sure what you mean by this. You seem to imply that if the `while` condition is false, then its side effects are not evaluated, but I don't see this happening. I think the `while` loop is the easiest one, you just evaluate (with side effects) the condition at the beginning and break if it is false, then run the body. Jumps `continue` and `break` automatically work properly, without doing anything else. The problem with `continue` is only with `do`-`while` and `for` loops, and it can be solved e.g. by copying the iteration (for `for` loops) or condition (for `do`-`while` loops) code before the `CONTINUE` jump. Jump `break` still needs no special treatment, I think. Unrolling the first iteration is also an option, I suppose, but it doesn't seem to make our life simpler. Or does it?
> Original solution to retroactively add instructions by scanning loop bodies?

Yeah. Apparently with `do`-`while` loops that's the only feasible solution.
> Why does it need a new jump type?

My understanding is that Zeb prefers to distinguish `CONTINUE` jumps that have already been replaced with iteration/condition code from those that are not. So the idea would be:
* While parsing the loop body, insert `ITER_AND_CONTINUE` jumps when `continue` statements appear;
* Then, once you know the iteration/condition code, scan again the generated code and replace every `ITER_AND_CONTINUE` jump with the iteration/condition code and a `CONTINUE` jump.

As I said, I don't have a strong opinion between this and what you originally did (i.e., using `CONTINUE` jumps for the get-go).

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37960