[PATCH v3 0/3] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- v3: 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
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 12 +++++++++++- libs/vkd3d-shader/hlsl_codegen.c | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0e07fe57..6b010493 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -5719,7 +5719,17 @@ statement: | loop_statement jump_statement: - KW_RETURN expr ';' + KW_BREAK ';' + { + struct hlsl_ir_node *jump; + + if (!($$ = make_empty_list(ctx))) + YYABORT; + if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_BREAK, &@1))) + YYABORT; + list_add_tail($$, &jump->entry); + } + | KW_RETURN expr ';' { if (!add_return(ctx, $2, node_from_list($2), &@1)) YYABORT; diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 765b1907..5c10ec8b 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -3978,6 +3978,37 @@ static bool type_has_object_components(struct hlsl_type *type) return false; } +static void validate_flow_control_usage(struct hlsl_ctx *ctx, const struct hlsl_block *block) +{ + struct hlsl_ir_node *instr; + + LIST_FOR_EACH_ENTRY(instr, &block->instrs, struct hlsl_ir_node, entry) + { + if (instr->type == HLSL_IR_LOOP) + continue; + + /* TODO: allow breaks in the switch statements. */ + + if (instr->type == HLSL_IR_IF) + { + struct hlsl_ir_if *iff = hlsl_ir_if(instr); + + validate_flow_control_usage(ctx, &iff->then_block); + validate_flow_control_usage(ctx, &iff->else_block); + } + else if (instr->type == HLSL_IR_JUMP) + { + struct hlsl_ir_jump *jump = hlsl_ir_jump(instr); + + if (jump->type == HLSL_IR_JUMP_BREAK) + { + hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "Flow control instruction \"break\" can't be used in this context."); + } + } + } +} + int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_func, enum vkd3d_shader_target_type target_type, struct vkd3d_shader_code *out) { @@ -4002,6 +4033,8 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry lower_return(ctx, entry_func, body, false); while (hlsl_transform_ir(ctx, lower_calls, body, NULL)); + /* Check that 'break'/'continue' are used in appropriate context. */ + validate_flow_control_usage(ctx, body); hlsl_transform_ir(ctx, lower_index_loads, body, NULL); -- 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.y | 69 ++++++++++++++++++++++++++++++-- libs/vkd3d-shader/hlsl_codegen.c | 6 ++- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 6b010493..f65c82c8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -454,10 +454,48 @@ 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 void transform_loop_body(struct hlsl_ctx *ctx, bool (*func)(struct hlsl_ctx *ctx, struct hlsl_ir_node *, void *), + struct hlsl_block *block, void *context) { - struct hlsl_block body_block; + struct hlsl_ir_node *instr, *next; + + 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); + + transform_loop_body(ctx, func, &iff->then_block, context); + transform_loop_body(ctx, func, &iff->else_block, context); + } + + func(ctx, instr, context); + } +} + +static bool prepend_continue_loop_iterator(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + struct hlsl_block *loop_iter = context; + struct hlsl_ir_jump *jump; + struct hlsl_block iter; + + if (instr->type != HLSL_IR_JUMP) + return false; + jump = hlsl_ir_jump(instr); + if (jump->type != HLSL_IR_JUMP_CONTINUE) + return false; + + if (!hlsl_clone_block(ctx, &iter, loop_iter)) + return false; + list_move_before(&instr->entry, &iter.instrs); + + return true; +} + +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) +{ + struct hlsl_block body_block, iter_block; struct hlsl_ir_node *loop; unsigned int i; @@ -498,6 +536,7 @@ static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const goto oom; hlsl_block_init(&body_block); + hlsl_block_init(&iter_block); if (type != LOOP_DO_WHILE) list_move_tail(&body_block.instrs, cond); @@ -505,11 +544,23 @@ 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); + { + struct hlsl_block block; + + list_move_tail(&iter_block.instrs, iter); + if (!hlsl_clone_block(ctx, &block, &iter_block)) + goto oom; + hlsl_block_add_block(&body_block, &block); + } if (type == LOOP_DO_WHILE) list_move_tail(&body_block.instrs, cond); + if (!list_empty(&iter_block.instrs)) + transform_loop_body(ctx, prepend_continue_loop_iterator, &body_block, &iter_block); + + hlsl_block_cleanup(&iter_block); + if (!(loop = hlsl_new_loop(ctx, &body_block, loc))) goto oom; list_add_tail(init, &loop->entry); @@ -5729,6 +5780,16 @@ jump_statement: YYABORT; list_add_tail($$, &jump->entry); } + | KW_CONTINUE ';' + { + struct hlsl_ir_node *jump; + + if (!($$ = make_empty_list(ctx))) + YYABORT; + if (!(jump = hlsl_new_jump(ctx, HLSL_IR_JUMP_CONTINUE, &@1))) + YYABORT; + list_add_tail($$, &jump->entry); + } | KW_RETURN expr ';' { if (!add_return(ctx, $2, node_from_list($2), &@1)) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 5c10ec8b..a7c4d237 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -4000,10 +4000,12 @@ static void validate_flow_control_usage(struct hlsl_ctx *ctx, const struct hlsl_ { struct hlsl_ir_jump *jump = hlsl_ir_jump(instr); - if (jump->type == HLSL_IR_JUMP_BREAK) + if (jump->type == HLSL_IR_JUMP_BREAK + || jump->type == HLSL_IR_JUMP_CONTINUE) { hlsl_error(ctx, &instr->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, - "Flow control instruction \"break\" can't be used in this context."); + "Flow control instruction \"%s\" can't be used in this context.", + jump->type == HLSL_IR_JUMP_BREAK ? "break" : "continue"); } } } -- 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 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index d066b13e..73e5ff8e 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -4780,6 +4780,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: { struct sm4_register *reg = &instr.srcs[0].reg; diff --git a/tests/hlsl-for.shader_test b/tests/hlsl-for.shader_test index b6896981..5e1cde76 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
On Fri Jun 23 20:49:48 2023 +0000, Zebediah Figura wrote:
My problem isn't so much with doing extra passes later, and in fact, given the complexity of this pass, I *probably* prefer this to be its own pass instead of something that's handled in create_loop(). My problem is that I want the IR to have a universally unambiguous meaning, so we can do things like dump it to stderr at any point, look at it, and check whether it's correct. I agree. Would it be feasible to immediately store somewhere in the context the iteration code when parsing `for (...; ...; ...)` (before parsing the body), and then immediately copy it each time it is needed while parsing the body, therefore removing the need for a double pass? That might require some refactoring, but shouldn't be too difficult.
Similarly, I don't like too much the fact that the `continue`/`break` validation is done in a different pass. The frontend should be responsible for creating valid IR in the first place. If you refactor the code the way I said you can avoid that, simply by keeping a boolean telling you whether you are in a loop or not. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_36962
On Tue Jun 27 13:15:42 2023 +0000, Giovanni Mascellani wrote:
I agree. Would it be feasible to immediately store somewhere in the context the iteration code when parsing `for (...; ...; ...)` (before parsing the body), and then immediately copy it each time it is needed while parsing the body, therefore removing the need for a double pass? That might require some refactoring, but shouldn't be too difficult. Similarly, I don't like too much the fact that the `continue`/`break` validation is done in a different pass. The frontend should be responsible for creating valid IR in the first place. If you refactor the code the way I said you can avoid that, simply by keeping a boolean telling you whether you are in a loop or not. 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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/245#note_37173
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)