[PATCH v6 0/4] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.
Signed-off-by: Nikolay Sivov <nsivov(a)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. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)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) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)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); +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)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; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)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) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_46807
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_47641
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_47642
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_47705
On Wed Oct 4 22:49:14 2023 +0000, Zebediah Figura wrote:
We might as well return void here. Good point.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_47706
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_47738
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)