[PATCH v6 0/4] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
-- v6: vkd3d-shader/tpf: Write 'continue' instruction. vkd3d-shader/hlsl: Handle 'continue' statements. vkd3d-shader/hlsl: Allow 'break' instructions in loops. tests: Add some tests for 'break'/'continue' in loops.
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- tests/hlsl/loop.shader_test | 59 +++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
diff --git a/tests/hlsl/loop.shader_test b/tests/hlsl/loop.shader_test index 75a31f7af..9f8bcb92d 100644 --- a/tests/hlsl/loop.shader_test +++ b/tests/hlsl/loop.shader_test @@ -41,3 +41,62 @@ float4 main() : sv_target uniform 0 float 4.0 draw quad probe all rgba (20.0, 20.0, 20.0, 20.0) + +[pixel shader todo] +float a; + +float4 main() : sv_target +{ + int i = 0; + float res = a; + + while (i < 10) + { + if (i == 5) + { + res += 0.1f; + break; + } + res += 1.0f; + i++; + if (i == 2) continue; + res += 100.f; + } + + return res; +} + +[test] +uniform 0 float 4.0 +todo draw quad +todo probe all rgba (409.1, 409.1, 409.1, 409.1) + +[pixel shader todo] +float a; + +float4 main() : sv_target +{ + int i = 0; + float res = a; + + do + { + res += 1.0f; + if (i == 5) + { + res += 0.1f; + break; + } + i++; + if (i == 2) continue; + res += 100.f; + } + while (i < 10); + + return res; +} + +[test] +uniform 0 float 4.0 +todo draw quad +todo probe all rgba (410.1, 410.1, 410.1, 410.1)
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl.y | 53 +++++++++++++++++++++++++++++++------- tests/hlsl/for.shader_test | 7 +++++ 3 files changed, 53 insertions(+), 9 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 44cebaaf6..9cf7c6ba7 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -702,6 +702,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 a47246de2..25de56d25 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4559,6 +4559,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 @@ -5264,6 +5272,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 @@ -6076,7 +6091,25 @@ 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, + "The 'break' statement must be used inside of a loop."); + } + + if (!($$ = make_empty_block(ctx))) + YYABORT; + if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_BREAK, NULL, &@1))) + YYABORT; + hlsl_block_add_instr($$, jump); + } + | KW_RETURN expr ';' { $$ = $2; if (!add_return(ctx, $$, node_from_block($$), &@1)) @@ -6165,22 +6198,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); +}
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.c | 4 +++ libs/vkd3d-shader/hlsl.h | 1 + libs/vkd3d-shader/hlsl.y | 69 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 5fe9047bf..cb5f19297 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2540,6 +2540,10 @@ static void dump_ir_jump(struct vkd3d_string_buffer *buffer, const struct hlsl_i case HLSL_IR_JUMP_RETURN: vkd3d_string_buffer_printf(buffer, "return"); break; + + case HLSL_IR_JUMP_LOOP_CONTINUE: + vkd3d_string_buffer_printf(buffer, "loop_continue"); + break; } }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 9cf7c6ba7..77914ab77 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -574,6 +574,7 @@ enum hlsl_ir_jump_type { HLSL_IR_JUMP_BREAK, HLSL_IR_JUMP_CONTINUE, + HLSL_IR_JUMP_LOOP_CONTINUE, HLSL_IR_JUMP_DISCARD_NEG, HLSL_IR_JUMP_DISCARD_NZ, HLSL_IR_JUMP_RETURN, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 25de56d25..b376c51e2 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -464,6 +464,55 @@ static bool attribute_list_has_duplicates(const struct parse_attribute_list *att return false; }
+static bool resolve_loop_continue(struct hlsl_ctx *ctx, struct hlsl_block *block, enum loop_type type, + struct hlsl_block *cond, struct hlsl_block *iter) +{ + struct hlsl_ir_node *instr, *next; + bool progress = false; + + LIST_FOR_EACH_ENTRY_SAFE(instr, next, &block->instrs, struct hlsl_ir_node, entry) + { + if (instr->type == HLSL_IR_IF) + { + struct hlsl_ir_if *iff = hlsl_ir_if(instr); + + progress |= resolve_loop_continue(ctx, &iff->then_block, type, cond, iter); + progress |= resolve_loop_continue(ctx, &iff->else_block, type, cond, iter); + } + else if (instr->type == HLSL_IR_JUMP) + { + struct hlsl_ir_jump *jump = hlsl_ir_jump(instr); + struct hlsl_block block; + + if (jump->type != HLSL_IR_JUMP_LOOP_CONTINUE) + continue; + + if (type == LOOP_DO_WHILE) + { + if (!hlsl_clone_block(ctx, &block, cond)) + return false; + if (!append_conditional_break(ctx, &block)) + { + hlsl_block_cleanup(&block); + return false; + } + list_move_before(&instr->entry, &block.instrs); + } + else if (type == LOOP_FOR) + { + if (!hlsl_clone_block(ctx, &block, iter)) + return false; + list_move_before(&instr->entry, &block.instrs); + } + jump->type = HLSL_IR_JUMP_CONTINUE; + + progress = true; + } + } + + return progress; +} + static struct hlsl_block *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const struct parse_attribute_list *attributes, struct hlsl_block *init, struct hlsl_block *cond, struct hlsl_block *iter, struct hlsl_block *body, const struct vkd3d_shader_location *loc) @@ -501,6 +550,8 @@ static struct hlsl_block *create_loop(struct hlsl_ctx *ctx, enum loop_type type, } }
+ resolve_loop_continue(ctx, body, type, cond, iter); + if (!init && !(init = make_empty_block(ctx))) goto oom;
@@ -6109,6 +6160,24 @@ jump_statement: YYABORT; hlsl_block_add_instr($$, jump); } + | KW_CONTINUE ';' + { + struct hlsl_ir_node *jump; + struct hlsl_scope *scope; + + if (!(scope = get_loop_scope(ctx->cur_scope))) + { + hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "The 'continue' statement must be used inside of a loop."); + } + + if (!($$ = make_empty_block(ctx))) + YYABORT; + + if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_LOOP_CONTINUE, NULL, &@1))) + YYABORT; + hlsl_block_add_instr($$, jump); + } | KW_RETURN expr ';' { $$ = $2;
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/tpf.c | 4 ++++ tests/hlsl/for.shader_test | 4 ++-- tests/hlsl/loop.shader_test | 12 ++++++------ 3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 58b7f030d..ae5bb93d6 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -5208,6 +5208,10 @@ static void write_sm4_jump(const struct tpf_writer *tpf, const struct hlsl_ir_ju 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) diff --git a/tests/hlsl/loop.shader_test b/tests/hlsl/loop.shader_test index 9f8bcb92d..bf26b69cf 100644 --- a/tests/hlsl/loop.shader_test +++ b/tests/hlsl/loop.shader_test @@ -42,7 +42,7 @@ uniform 0 float 4.0 draw quad probe all rgba (20.0, 20.0, 20.0, 20.0)
-[pixel shader todo] +[pixel shader] float a;
float4 main() : sv_target @@ -68,10 +68,10 @@ float4 main() : sv_target
[test] uniform 0 float 4.0 -todo draw quad -todo probe all rgba (409.1, 409.1, 409.1, 409.1) +draw quad +probe all rgba (409.1, 409.1, 409.1, 409.1)
-[pixel shader todo] +[pixel shader] float a;
float4 main() : sv_target @@ -98,5 +98,5 @@ float4 main() : sv_target
[test] uniform 0 float 4.0 -todo draw quad -todo probe all rgba (410.1, 410.1, 410.1, 410.1) +draw quad +probe all rgba (410.1, 410.1, 410.1, 410.1)
On Tue Sep 26 13:29:55 2023 +0000, Giovanni Mascellani wrote:
The code seems fine. But I don't think we have any test using `continue` or `break` inside `while` or `do`-`while` loops. Please add them and then it's good to go for me.
I added some tests, and that showed broken logic in do-while. For some reason I used condition over 'continue' in there, instead of breaking out conditionally, before 'continue'. This is fixed now.
This merge request was approved by Giovanni Mascellani.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
{ HLSL_IR_JUMP_BREAK, HLSL_IR_JUMP_CONTINUE,
- HLSL_IR_JUMP_LOOP_CONTINUE,
I don't love these names, although I'm not sure I have better suggestions (CONTINUE_WITH_ITER maybe?)
As long as the naming is unclear, which is probably unavoidable, I think we need some documentation here describing which is which.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
return false;}
+static bool resolve_loop_continue(struct hlsl_ctx *ctx, struct hlsl_block *block, enum loop_type type,
struct hlsl_block *cond, struct hlsl_block *iter)
We might as well return void here.
On Wed Oct 4 22:49:13 2023 +0000, Zebediah Figura wrote:
I don't love these names, although I'm not sure I have better suggestions (CONTINUE_WITH_ITER maybe?) As long as the naming is unclear, which is probably unavoidable, I think we need some documentation here describing which is which.
Yes, some comments are useful here. Regarding the name, it's not only for iterator part, it depends on a loop kind, so calling it WITH_ITER will make it even less accurate. The name should make it clear that it's some jump that's not going to survive for long, and is not useful by itself. Maybe append INTERNAL, or something similar? I'm definitely open to suggestions.
On Wed Oct 4 22:49:14 2023 +0000, Zebediah Figura wrote:
We might as well return void here.
Good point.
On Thu Oct 5 13:54:37 2023 +0000, Nikolay Sivov wrote:
Yes, some comments are useful here. Regarding the name, it's not only for iterator part, it depends on a loop kind, so calling it WITH_ITER will make it even less accurate. The name should make it clear that it's some jump that's not going to survive for long, and is not useful by itself. Maybe append INTERNAL, or something similar? I'm definitely open to suggestions.
LOOP_CONTINUE is also questionable, because 'continue' does not belong anywhere else but loops.
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)