[PATCH v2 0/5] MR245: vkd3d-shader/hlsl: Add support for 'break' and 'continue' jumps in loops.
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
-- v2: vkd3d-shader/tpf: Write 'continue' instruction. vkd3d-shader: Insert loop iterator block before 'continue' jumps. vkd3d-shader/hlsl: Parse 'continue' statement. vkd3d-shader: Keep loop iterator instructions as a block. vkd3d-shader/hlsl: Allow 'break' instructions in loops.
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.y | 12 +++++++++++- libs/vkd3d-shader/hlsl_codegen.c | 26 ++++++++++++++++++++++++++ 2 files changed, 37 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..e0d92cb3 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -3978,6 +3978,30 @@ 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_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 +4026,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);
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.c | 19 ++++++++++++++----- libs/vkd3d-shader/hlsl.h | 3 ++- libs/vkd3d-shader/hlsl.y | 12 ++++++++---- 3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index ba5bcfbf..996feb85 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1443,8 +1443,8 @@ struct hlsl_ir_node *hlsl_new_jump(struct hlsl_ctx *ctx, enum hlsl_ir_jump_type return &jump->node; }
-struct hlsl_ir_node *hlsl_new_loop(struct hlsl_ctx *ctx, - struct hlsl_block *block, const struct vkd3d_shader_location *loc) +struct hlsl_ir_node *hlsl_new_loop(struct hlsl_ctx *ctx, struct hlsl_block *body, + struct hlsl_block *iter, const struct vkd3d_shader_location *loc) { struct hlsl_ir_loop *loop;
@@ -1452,7 +1452,9 @@ struct hlsl_ir_node *hlsl_new_loop(struct hlsl_ctx *ctx, return NULL; init_node(&loop->node, HLSL_IR_LOOP, NULL, loc); hlsl_block_init(&loop->body); - hlsl_block_add_block(&loop->body, block); + hlsl_block_add_block(&loop->body, body); + hlsl_block_init(&loop->iter); + hlsl_block_add_block(&loop->iter, iter); return &loop->node; }
@@ -1608,15 +1610,21 @@ static struct hlsl_ir_node *clone_load(struct hlsl_ctx *ctx, struct clone_instr_
static struct hlsl_ir_node *clone_loop(struct hlsl_ctx *ctx, struct clone_instr_map *map, struct hlsl_ir_loop *src) { + struct hlsl_block body, iter; struct hlsl_ir_node *dst; - struct hlsl_block body;
if (!clone_block(ctx, &body, &src->body, map)) return NULL; + if (!clone_block(ctx, &iter, &src->iter, map)) + { + hlsl_block_cleanup(&body); + return NULL; + }
- if (!(dst = hlsl_new_loop(ctx, &body, &src->node.loc))) + if (!(dst = hlsl_new_loop(ctx, &body, &iter, &src->node.loc))) { hlsl_block_cleanup(&body); + hlsl_block_cleanup(&iter); return NULL; } return dst; @@ -2715,6 +2723,7 @@ static void free_ir_load(struct hlsl_ir_load *load) static void free_ir_loop(struct hlsl_ir_loop *loop) { hlsl_block_cleanup(&loop->body); + hlsl_block_cleanup(&loop->iter); vkd3d_free(loop); }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index a797ef6a..2c2a6308 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -489,6 +489,7 @@ struct hlsl_ir_loop struct hlsl_ir_node node; /* loop condition is stored in the body (as "if (!condition) break;") */ struct hlsl_block body; + struct hlsl_block iter; unsigned int next_index; /* liveness index of the end of the loop */ };
@@ -1145,7 +1146,7 @@ bool hlsl_index_is_resource_access(struct hlsl_ir_index *index); struct hlsl_ir_node *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc); struct hlsl_ir_node *hlsl_new_loop(struct hlsl_ctx *ctx, - struct hlsl_block *block, const struct vkd3d_shader_location *loc); + struct hlsl_block *body, struct hlsl_block *iter, const struct vkd3d_shader_location *loc); struct hlsl_ir_node *hlsl_new_resource_load(struct hlsl_ctx *ctx, const struct hlsl_resource_load_params *params, const struct vkd3d_shader_location *loc); struct hlsl_ir_node *hlsl_new_resource_store(struct hlsl_ctx *ctx, const struct hlsl_deref *resource, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 6b010493..9a72fdf5 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -457,7 +457,7 @@ static bool attribute_list_has_duplicates(const struct parse_attribute_list *att 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; + struct hlsl_block body_block, iter_block; struct hlsl_ir_node *loop; unsigned int i;
@@ -476,7 +476,7 @@ static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const } else { - hlsl_warning(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Loop unrolling is not implemented.\n"); + hlsl_warning(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Loop unrolling is not implemented."); } } else if (!strcmp(attr->name, "loop") @@ -498,6 +498,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,12 +506,15 @@ 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); + { + list_move_tail(&iter_block.instrs, iter); + hlsl_block_add_block(&body_block, &iter_block); + }
if (type == LOOP_DO_WHILE) list_move_tail(&body_block.instrs, cond);
- if (!(loop = hlsl_new_loop(ctx, &body_block, loc))) + if (!(loop = hlsl_new_loop(ctx, &body_block, &iter_block, loc))) goto oom; list_add_tail(init, &loop->entry);
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.y | 10 ++++++++++ libs/vkd3d-shader/hlsl_codegen.c | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 9a72fdf5..d9cbe72f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -5733,6 +5733,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 e0d92cb3..6f2ff006 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -3993,10 +3993,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"); } } }
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/hlsl.y | 6 ++- libs/vkd3d-shader/hlsl_codegen.c | 69 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index d9cbe72f..3d7faabf 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -507,8 +507,12 @@ static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const
if (iter) { + struct hlsl_block block; + list_move_tail(&iter_block.instrs, iter); - hlsl_block_add_block(&body_block, &iter_block); + if (!hlsl_clone_block(ctx, &block, &iter_block)) + goto oom; + hlsl_block_add_block(&body_block, &block); }
if (type == LOOP_DO_WHILE) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 6f2ff006..45c12eda 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -4004,6 +4004,74 @@ static void validate_flow_control_usage(struct hlsl_ctx *ctx, const struct hlsl_ } }
+static bool prepend_continue_loop_iterator(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + struct hlsl_ir_loop *loop = context; + struct hlsl_ir_jump *jump; + struct hlsl_block iter; + + if (list_empty(&loop->iter.instrs)) + return false; + 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 bool hlsl_transform_loop(struct hlsl_ctx *ctx, bool (*func)(struct hlsl_ctx *ctx, struct hlsl_ir_node *, void *), + struct hlsl_block *block, void *context) +{ + 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 |= hlsl_transform_loop(ctx, func, &iff->then_block, context); + progress |= hlsl_transform_loop(ctx, func, &iff->else_block, context); + } + + progress |= func(ctx, instr, context); + } + + return progress; +} + +static void foreach_loop(struct hlsl_ctx *ctx, bool (*func)(struct hlsl_ctx *ctx, struct hlsl_ir_node *, void *), + struct hlsl_block *body) +{ + struct hlsl_ir_node *instr, *next; + + LIST_FOR_EACH_ENTRY_SAFE(instr, next, &body->instrs, struct hlsl_ir_node, entry) + { + if (instr->type == HLSL_IR_IF) + { + struct hlsl_ir_if *iff = hlsl_ir_if(instr); + + foreach_loop(ctx, func, &iff->then_block); + foreach_loop(ctx, func, &iff->else_block); + } + else if (instr->type == HLSL_IR_LOOP) + { + struct hlsl_ir_loop *loop = hlsl_ir_loop(instr); + + hlsl_transform_loop(ctx, func, &loop->body, loop); + + foreach_loop(ctx, func, &loop->body); + } + } +} + 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) { @@ -4030,6 +4098,7 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry while (hlsl_transform_ir(ctx, lower_calls, body, NULL)); /* Check that 'break'/'continue' are used in appropriate context. */ validate_flow_control_usage(ctx, body); + foreach_loop(ctx, prepend_continue_loop_iterator, body);
hlsl_transform_ir(ctx, lower_index_loads, body, NULL);
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 ++-- 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)
Comment on v5 since Gitlab is being uncoöperative: the foreach_loop() function is currently applying logic twice to "if" inside "for", but moreover, I would leave off adding a generic wrapper for this until we have more passes that need it. I.e. I'd just write a single function that calls itself.
More generally, I'm not thrilled about the way this gives the same IR two different semantics depending on whether this iterator pass has been done yet. I suppose that an improvement would be to use two different jump-type enums, but I don't immediately see how to name them.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
/* TODO: allow breaks in the switch statements. */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.");}}- }
+}
I don't think this function does what you want it to do.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
if (iter) {
struct hlsl_block block;list_move_tail(&iter_block.instrs, iter);
hlsl_block_add_block(&body_block, &iter_block);
if (!hlsl_clone_block(ctx, &block, &iter_block))goto oom;hlsl_block_add_block(&body_block, &block);
This should really be a part of whatever commit introduces iter_block, i.e. 2/5.
On Fri Jun 23 20:41:40 2023 +0000, Zebediah Figura wrote:
Comment on v5 since Gitlab is being uncoöperative: the foreach_loop() function is currently applying logic twice to "if" inside "for", but moreover, I would leave off adding a generic wrapper for this until we have more passes that need it. I.e. I'd just write a single function that calls itself. More generally, I'm not thrilled about the way this gives the same IR two different semantics depending on whether this iterator pass has been done yet. I suppose that an improvement would be to use two different jump-type enums, but I don't immediately see how to name them.
It doesn't really change 'continue' semantics, but I agree the fact that extra work is needed to fix that stuff up later is not ideal. Now I think that maybe inserting iterator right in create_loop(), descending into 'if' blocks, is better. It will only need to handle 'if's, and 'switch'es when we have them.
On Fri Jun 23 20:41:40 2023 +0000, Nikolay Sivov wrote:
It doesn't really change 'continue' semantics, but I agree the fact that extra work is needed to fix that stuff up later is not ideal. Now I think that maybe inserting iterator right in create_loop(), descending into 'if' blocks, is better. It will only need to handle 'if's, and 'switch'es when we have them.
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.
-
Nikolay Sivov -
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)