[Mesa-dev] [PATCH 2/2] panfrost: Implement custom block/instruction iterators
Alyssa Rosenzweig
alyssa.rosenzweig at collabora.com
Tue Aug 13 19:59:03 UTC 2019
Could you explain a little bit more why this is neded? What do these
helpers do that the generic list.h helpers can't? Would it be possible
to extend the shared list.h helpers (might this contain functionality
interesting to other drivers as well)?
What about the new next_ins argument -- should that always be required
to specify? Maybe we want a dedicated variant _with_next() to save the
empty argument in the usual case (when we don't actually need it).
On Tue, Aug 13, 2019 at 09:25:45PM +0200, Boris Brezillon wrote:
> The generic list helpers are too restrictive for us: we want to be
> able to update the instruction pointer within the foreach body, and the
> list_assert() check done in list_for_each_entry() prevents it.
> Sometimes we also want to update the next_ins pointer (in case we
> delete/replace the next instruction by something else).
>
> Let's implement our own iterators (still based on the existing list
> helpers) to address this limitation.
>
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> ---
> src/panfrost/midgard/compiler.h | 75 ++++++++++++++-----
> src/panfrost/midgard/midgard_compile.c | 2 +-
> src/panfrost/midgard/midgard_derivatives.c | 4 +-
> src/panfrost/midgard/midgard_liveness.c | 1 +
> src/panfrost/midgard/midgard_opt_copy_prop.c | 2 +-
> src/panfrost/midgard/midgard_opt_dce.c | 6 +-
> src/panfrost/midgard/midgard_opt_invert.c | 4 +-
> .../midgard/midgard_opt_perspective.c | 4 +-
> src/panfrost/midgard/midgard_ra.c | 4 +-
> src/panfrost/midgard/midgard_schedule.c | 18 +++--
> src/panfrost/midgard/mir_promote_uniforms.c | 2 +-
> 11 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/src/panfrost/midgard/compiler.h b/src/panfrost/midgard/compiler.h
> index d62157f3be4f..19f65ddc96ec 100644
> --- a/src/panfrost/midgard/compiler.h
> +++ b/src/panfrost/midgard/compiler.h
> @@ -305,44 +305,83 @@ mir_remove_instruction(struct midgard_instruction *ins)
> list_del(&ins->link);
> }
>
> -static inline midgard_instruction*
> -mir_prev_op(struct midgard_instruction *ins)
> +static inline midgard_instruction *
> +mir_first_op(struct midgard_block *block)
> {
> - return list_last_entry(&(ins->link), midgard_instruction, link);
> + assert(block);
> + return list_first_entry(&block->instructions, struct midgard_instruction, link);
> }
>
> -static inline midgard_instruction*
> +static inline midgard_instruction *
> +mir_last_op(struct midgard_block *block)
> +{
> + assert(block);
> + return list_last_entry(&block->instructions, struct midgard_instruction, link);
> +}
> +
> +static inline midgard_instruction *
> +mir_prev_op(struct midgard_instruction *ins)
> +{
> + assert(ins);
> + return list_last_entry(&ins->link, struct midgard_instruction, link);
> +}
> +
> +static inline midgard_instruction *
> mir_next_op(struct midgard_instruction *ins)
> {
> - return list_first_entry(&(ins->link), midgard_instruction, link);
> + assert(ins);
> + return list_first_entry(&ins->link, struct midgard_instruction, link);
> +}
> +
> +static inline struct midgard_block *mir_first_block(compiler_context *ctx)
> +{
> + assert(ctx);
> + return list_first_entry(&ctx->blocks, struct midgard_block, link);
> +}
> +
> +static inline struct midgard_block *mir_next_block(struct midgard_block *b)
> +{
> + assert(b);
> + return list_first_entry(&b->link, struct midgard_block, link);
> }
>
> #define mir_foreach_block(ctx, v) \
> - list_for_each_entry(struct midgard_block, v, &ctx->blocks, link)
> + for (struct midgard_block *v = mir_first_block(ctx); \
> + &v->link != &ctx->blocks; v = mir_next_block(v))
>
> #define mir_foreach_block_from(ctx, from, v) \
> - list_for_each_entry_from(struct midgard_block, v, from, &ctx->blocks, link)
> + for (struct midgard_block *v = from; &v->link != &ctx->blocks; \
> + v = mir_next_block(v))
>
> #define mir_foreach_instr(ctx, v) \
> - list_for_each_entry(struct midgard_instruction, v, &ctx->current_block->instructions, link)
> + for (struct midgard_instruction *v = mir_first_op(ctx->current_block); \
> + &v->link != &ctx->current_block->instructions; v = mir_next_op(v))
>
> -#define mir_foreach_instr_safe(ctx, v) \
> - list_for_each_entry_safe(struct midgard_instruction, v, &ctx->current_block->instructions, link)
> +#define mir_foreach_instr_safe(ctx, v, n) \
> + for (struct midgard_instruction *v = mir_first_op(ctx->current_block), \
> + struct midgard_instruction *n = mir_next_op(v); \
> + &v->link != &ctx->current_block->instructions; \
> + v = n, n = mir_next_op(v))
>
> #define mir_foreach_instr_in_block(block, v) \
> - list_for_each_entry(struct midgard_instruction, v, &block->instructions, link)
> + for (struct midgard_instruction *v = mir_first_op(block); \
> + &v->link != &block->instructions; v = mir_next_op(v))
>
> -#define mir_foreach_instr_in_block_safe(block, v) \
> - list_for_each_entry_safe(struct midgard_instruction, v, &block->instructions, link)
> +#define mir_foreach_instr_in_block_safe(block, v, n) \
> + for (struct midgard_instruction *v = mir_first_op(block), *n = mir_next_op(v); \
> + &v->link != &block->instructions; v = n, n = mir_next_op(v))
>
> #define mir_foreach_instr_in_block_safe_rev(block, v) \
> - list_for_each_entry_safe_rev(struct midgard_instruction, v, &block->instructions, link)
> + for (struct midgard_instruction *v = mir_last_op(block), *p = mir_prev_op(v); \
> + &v->link != &block->instructions; v = p, p = mir_prev_op(v))
>
> #define mir_foreach_instr_in_block_from(block, v, from) \
> - list_for_each_entry_from(struct midgard_instruction, v, from, &block->instructions, link)
> + for (struct midgard_instruction *v = from; \
> + &v->link != &block->instructions; v = mir_next_op(v))
>
> #define mir_foreach_instr_in_block_from_rev(block, v, from) \
> - list_for_each_entry_from_rev(struct midgard_instruction, v, from, &block->instructions, link)
> + for (struct midgard_instruction *v = from, *p = mir_prev_op(v); \
> + &v->link != &block->instructions; v = p, p = mir_prev_op(v))
>
> #define mir_foreach_bundle_in_block(block, v) \
> util_dynarray_foreach(&block->bundles, midgard_bundle, v)
> @@ -351,9 +390,9 @@ mir_next_op(struct midgard_instruction *ins)
> mir_foreach_block(ctx, v_block) \
> mir_foreach_instr_in_block(v_block, v)
>
> -#define mir_foreach_instr_global_safe(ctx, v) \
> +#define mir_foreach_instr_global_safe(ctx, v, n) \
> mir_foreach_block(ctx, v_block) \
> - mir_foreach_instr_in_block_safe(v_block, v)
> + mir_foreach_instr_in_block_safe(v_block, v, n)
>
> static inline midgard_instruction *
> mir_last_in_block(struct midgard_block *block)
> diff --git a/src/panfrost/midgard/midgard_compile.c b/src/panfrost/midgard/midgard_compile.c
> index e261e80c06c5..aa97a2ac6b8e 100644
> --- a/src/panfrost/midgard/midgard_compile.c
> +++ b/src/panfrost/midgard/midgard_compile.c
> @@ -2116,7 +2116,7 @@ midgard_opt_cull_dead_branch(compiler_context *ctx, midgard_block *block)
> {
> bool branched = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (!midgard_is_branch_unit(ins->unit)) continue;
>
> /* We ignore prepacked branches since the fragment epilogue is
> diff --git a/src/panfrost/midgard/midgard_derivatives.c b/src/panfrost/midgard/midgard_derivatives.c
> index 0f15af3db427..6717063aae3d 100644
> --- a/src/panfrost/midgard/midgard_derivatives.c
> +++ b/src/panfrost/midgard/midgard_derivatives.c
> @@ -122,7 +122,7 @@ midgard_emit_derivatives(compiler_context *ctx, nir_alu_instr *instr)
> void
> midgard_lower_derivatives(compiler_context *ctx, midgard_block *block)
> {
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_TEXTURE_4) continue;
> if (!OP_IS_DERIVATIVE(ins->texture.op)) continue;
>
> @@ -150,7 +150,7 @@ midgard_lower_derivatives(compiler_context *ctx, midgard_block *block)
> dup.texture.in_reg_swizzle = SWIZZLE_ZWWW;
>
> /* Insert the new instruction */
> - mir_insert_instruction_before(mir_next_op(ins), dup);
> + mir_insert_instruction_before(next_ins, dup);
>
> /* TODO: Set .cont/.last automatically via dataflow analysis */
> ctx->texture_op_count++;
> diff --git a/src/panfrost/midgard/midgard_liveness.c b/src/panfrost/midgard/midgard_liveness.c
> index 8ecb22ee2739..9ae47ac4f513 100644
> --- a/src/panfrost/midgard/midgard_liveness.c
> +++ b/src/panfrost/midgard/midgard_liveness.c
> @@ -75,6 +75,7 @@ mir_is_live_after(compiler_context *ctx, midgard_block *block, midgard_instructi
> {
> /* Check the rest of the block for liveness */
>
> + assert(&start->link != &block->instructions);
> mir_foreach_instr_in_block_from(block, ins, mir_next_op(start)) {
> if (mir_has_arg(ins, src))
> return true;
> diff --git a/src/panfrost/midgard/midgard_opt_copy_prop.c b/src/panfrost/midgard/midgard_opt_copy_prop.c
> index dc5579c4d463..183e3d3e4460 100644
> --- a/src/panfrost/midgard/midgard_opt_copy_prop.c
> +++ b/src/panfrost/midgard/midgard_opt_copy_prop.c
> @@ -30,7 +30,7 @@ midgard_opt_copy_prop(compiler_context *ctx, midgard_block *block)
> {
> bool progress = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_ALU_4) continue;
> if (!OP_IS_MOVE(ins->alu.op)) continue;
>
> diff --git a/src/panfrost/midgard/midgard_opt_dce.c b/src/panfrost/midgard/midgard_opt_dce.c
> index 9964675763c2..caf3ad45134b 100644
> --- a/src/panfrost/midgard/midgard_opt_dce.c
> +++ b/src/panfrost/midgard/midgard_opt_dce.c
> @@ -31,7 +31,7 @@ midgard_opt_dead_code_eliminate(compiler_context *ctx, midgard_block *block)
> {
> bool progress = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_ALU_4) continue;
> if (ins->compact_branch) continue;
>
> @@ -54,7 +54,7 @@ midgard_opt_dead_move_eliminate(compiler_context *ctx, midgard_block *block)
> {
> bool progress = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_ALU_4) continue;
> if (ins->compact_branch) continue;
> if (!OP_IS_MOVE(ins->alu.op)) continue;
> @@ -93,7 +93,7 @@ midgard_opt_dead_move_eliminate(compiler_context *ctx, midgard_block *block)
> void
> midgard_opt_post_move_eliminate(compiler_context *ctx, midgard_block *block, struct ra_graph *g)
> {
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_ALU_4) continue;
> if (ins->compact_branch) continue;
> if (!OP_IS_MOVE(ins->alu.op)) continue;
> diff --git a/src/panfrost/midgard/midgard_opt_invert.c b/src/panfrost/midgard/midgard_opt_invert.c
> index 422be6ef03e5..e46b44ec06a3 100644
> --- a/src/panfrost/midgard/midgard_opt_invert.c
> +++ b/src/panfrost/midgard/midgard_opt_invert.c
> @@ -31,7 +31,7 @@
> void
> midgard_lower_invert(compiler_context *ctx, midgard_block *block)
> {
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_ALU_4) continue;
> if (!ins->invert) continue;
>
> @@ -58,7 +58,7 @@ midgard_lower_invert(compiler_context *ctx, midgard_block *block)
>
> ins->ssa_args.dest = temp;
> ins->invert = false;
> - mir_insert_instruction_before(mir_next_op(ins), not);
> + mir_insert_instruction_before(next_ins, not);
> }
> }
>
> diff --git a/src/panfrost/midgard/midgard_opt_perspective.c b/src/panfrost/midgard/midgard_opt_perspective.c
> index 993000923b93..8d2f2a235590 100644
> --- a/src/panfrost/midgard/midgard_opt_perspective.c
> +++ b/src/panfrost/midgard/midgard_opt_perspective.c
> @@ -42,7 +42,7 @@ midgard_opt_combine_projection(compiler_context *ctx, midgard_block *block)
> {
> bool progress = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> /* First search for fmul */
> if (ins->type != TAG_ALU_4) continue;
> if (ins->alu.op != midgard_alu_op_fmul) continue;
> @@ -141,7 +141,7 @@ midgard_opt_varying_projection(compiler_context *ctx, midgard_block *block)
> {
> bool progress = false;
>
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> /* Search for a projection */
> if (ins->type != TAG_LOAD_STORE_4) continue;
> if (!OP_IS_PROJECTION(ins->load_store.op)) continue;
> diff --git a/src/panfrost/midgard/midgard_ra.c b/src/panfrost/midgard/midgard_ra.c
> index 4754971acb2a..46fc5263e395 100644
> --- a/src/panfrost/midgard/midgard_ra.c
> +++ b/src/panfrost/midgard/midgard_ra.c
> @@ -481,7 +481,7 @@ mir_lower_special_reads(compiler_context *ctx)
> /* Insert move before each read/write, depending on the
> * hazard we're trying to account for */
>
> - mir_foreach_instr_global_safe(ctx, pre_use) {
> + mir_foreach_instr_global_safe(ctx, pre_use, use) {
> if (pre_use->type != classes[j])
> continue;
>
> @@ -494,8 +494,6 @@ mir_lower_special_reads(compiler_context *ctx)
> }
>
> if (hazard_write) {
> - midgard_instruction *use = mir_next_op(pre_use);
> - assert(use);
> mir_insert_instruction_before(use, m);
> mir_rewrite_index_dst_tag(ctx, i, idx, classes[j]);
> } else {
> diff --git a/src/panfrost/midgard/midgard_schedule.c b/src/panfrost/midgard/midgard_schedule.c
> index 7ec0d4428a43..97b7ce7936f8 100644
> --- a/src/panfrost/midgard/midgard_schedule.c
> +++ b/src/panfrost/midgard/midgard_schedule.c
> @@ -556,6 +556,7 @@ schedule_bundle(compiler_context *ctx, midgard_block *block, midgard_instruction
>
> midgard_instruction *uins = ins;
> for (; packed_idx < bundle.instruction_count; ++packed_idx) {
> + assert(&uins->link != &block->instructions);
> bundle.instructions[packed_idx] = uins;
> uins = mir_next_op(uins);
> }
> @@ -586,8 +587,10 @@ schedule_block(compiler_context *ctx, midgard_block *block)
> ctx->blend_constant_offset = quadwords_within_block * 0x10;
> }
>
> - while(skip--)
> + while(skip--) {
> + assert(&ins->link != &block->instructions);
> ins = mir_next_op(ins);
> + }
>
> block->quadword_count += quadword_size(bundle.tag);
> }
> @@ -600,7 +603,7 @@ schedule_block(compiler_context *ctx, midgard_block *block)
> static void
> midgard_pair_load_store(compiler_context *ctx, midgard_block *block)
> {
> - mir_foreach_instr_in_block_safe(block, ins) {
> + mir_foreach_instr_in_block_safe(block, ins, next_ins) {
> if (ins->type != TAG_LOAD_STORE_4) continue;
>
> /* We've found a load/store op. Check if next is also load/store. */
> @@ -634,6 +637,9 @@ midgard_pair_load_store(compiler_context *ctx, midgard_block *block)
>
> /* We found one! Move it up to pair and remove it from the old location */
>
> + if (c == next_ins)
> + next_ins = mir_next_op(c);
> +
> mir_insert_instruction_before(ins, *c);
> mir_remove_instruction(c);
>
> @@ -782,7 +788,7 @@ static void mir_spill_register(
> * implicitly. For special writes, spill to a work register */
>
> if (!is_special || is_special_w) {
> - mir_foreach_instr_global_safe(ctx, ins) {
> + mir_foreach_instr_global_safe(ctx, ins, next_ins) {
> if (ins->ssa_args.dest != spill_node) continue;
>
> midgard_instruction st;
> @@ -796,7 +802,7 @@ static void mir_spill_register(
> st = v_load_store_scratch(ins->ssa_args.dest, spill_slot, true, ins->mask);
> }
>
> - spill_move = mir_insert_instruction_before(mir_next_op(ins), st);
> + spill_move = mir_insert_instruction_before(next_ins, st);
>
> if (!is_special)
> ctx->spills++;
> @@ -844,8 +850,10 @@ static void mir_spill_register(
> midgard_instruction *before = ins;
>
> /* For a csel, go back one more not to break up the bundle */
> - if (ins->type == TAG_ALU_4 && OP_IS_CSEL(ins->alu.op))
> + if (ins->type == TAG_ALU_4 && OP_IS_CSEL(ins->alu.op)) {
> before = mir_prev_op(before);
> + assert(&before->link != &block->instructions);
> + }
>
> midgard_instruction st;
>
> diff --git a/src/panfrost/midgard/mir_promote_uniforms.c b/src/panfrost/midgard/mir_promote_uniforms.c
> index 9f5be37be2cf..7dc09fcdf147 100644
> --- a/src/panfrost/midgard/mir_promote_uniforms.c
> +++ b/src/panfrost/midgard/mir_promote_uniforms.c
> @@ -39,7 +39,7 @@
> void
> midgard_promote_uniforms(compiler_context *ctx, unsigned promoted_count)
> {
> - mir_foreach_instr_global_safe(ctx, ins) {
> + mir_foreach_instr_global_safe(ctx, ins, next_ins) {
> if (ins->type != TAG_LOAD_STORE_4) continue;
> if (!OP_IS_UBO_READ(ins->load_store.op)) continue;
>
> --
> 2.21.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190813/fce8ca2f/attachment.sig>
More information about the mesa-dev
mailing list