[PATCH v5 0/3] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
-- v5: vkd3d-shader/tpf: Write 'continue' instruction. vkd3d-shader/hlsl: Handle 'continue' statements. vkd3d-shader/hlsl: Allow 'break' instructions in loops.
From: 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 fb6d485ea..ac038ec5f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4544,6 +4544,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 @@ -5249,6 +5257,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 @@ -6061,7 +6076,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)) @@ -6150,22 +6183,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
--- libs/vkd3d-shader/hlsl.c | 4 ++ libs/vkd3d-shader/hlsl.h | 1 + libs/vkd3d-shader/hlsl.y | 81 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 3 deletions(-)
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 ac038ec5f..d81feb03f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -415,7 +415,7 @@ static DWORD add_modifiers(struct hlsl_ctx *ctx, DWORD modifiers, DWORD mod, return modifiers | mod; }
-static bool append_conditional_break(struct hlsl_ctx *ctx, struct hlsl_block *cond_block) +static bool append_conditional_jump(struct hlsl_ctx *ctx, struct hlsl_block *cond_block, enum hlsl_ir_jump_type jump_type) { struct hlsl_ir_node *condition, *not, *iff, *jump; struct hlsl_block then_block; @@ -431,7 +431,7 @@ static bool append_conditional_break(struct hlsl_ctx *ctx, struct hlsl_block *co
hlsl_block_init(&then_block);
- if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_BREAK, NULL, &condition->loc))) + if (!(jump = hlsl_new_jump(ctx, jump_type, NULL, &condition->loc))) return false; hlsl_block_add_instr(&then_block, jump);
@@ -464,6 +464,61 @@ 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_WHILE) + { + jump->type = HLSL_IR_JUMP_CONTINUE; + } + else if (type == LOOP_DO_WHILE) + { + if (!hlsl_clone_block(ctx, &block, cond)) + return false; + if (!append_conditional_jump(ctx, &block, HLSL_IR_JUMP_CONTINUE)) + { + hlsl_block_cleanup(&block); + return false; + } + list_move_before(&instr->entry, &block.instrs); + list_remove(&instr->entry); + hlsl_free_instr(instr); + } + 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,10 +556,12 @@ 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;
- if (!append_conditional_break(ctx, cond)) + if (!append_conditional_jump(ctx, cond, HLSL_IR_JUMP_BREAK)) goto oom;
if (iter) @@ -6094,6 +6151,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
--- 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 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)
On Wed Jul 5 09:07:20 2023 +0000, Giovanni Mascellani wrote:
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).
Pushed something for all of this, please take a look.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
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)))
Minor, but it doesn't look you need to keep the scope in a variable.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
if (!hlsl_clone_block(ctx, &block, cond))return false;if (!append_conditional_jump(ctx, &block, HLSL_IR_JUMP_CONTINUE)){hlsl_block_cleanup(&block);return false;}list_move_before(&instr->entry, &block.instrs);list_remove(&instr->entry);hlsl_free_instr(instr);}else if (type == LOOP_FOR){if (!hlsl_clone_block(ctx, &block, iter))return false;list_move_before(&instr->entry, &block.instrs);
I wonder whether those `list_*()` calls should also be masked behind a layer of `hlsl_block_*()` helpers, but that doesn't need to happen in this MR.
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.
On Tue Sep 26 09:01:02 2023 +0000, Giovanni Mascellani wrote:
I wonder whether those `list_*()` calls should also be masked behind a layer of `hlsl_block_*()` helpers, but that doesn't need to happen in this MR.
Maybe, but iteration happens on nodes, and the idea here to insert before given node. I don't think we have block helpers for that.
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)