[2/2] panfrost: Implement custom block/instruction iterators

Submitted by Boris Brezillon on Aug. 13, 2019, 7:25 p.m.

Details

Message ID 20190813192545.13349-2-boris.brezillon@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Aug. 13, 2019, 7:25 p.m.
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@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(-)

Patch hide | download patch | download mbox

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;
 

Comments


On Tue, 13 Aug 2019 12:59:03 -0700
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> wrote:

> Could you explain a little bit more why this is neded? What do these
> helpers do that the generic list.h helpers can't?

The _safe attribute in generic helpers is just about handling the case
where the current item is removed from the list (or a new one is
inserted and you want to skip the new entry in your iteration). Some
users of the mir_foreach_xxx() iterators break this rule: they remove
entries beyond the current one. That's okay if the removed entry is
after the one stored in the temporary 'storage' param (also called
next/prev sometimes), but when one actually removes the next/prev
entry, the safe iterator ends up manipulating an element that's been
removed from the list (->{prev,next} might be invalid) or even worse,
something that's been released (use-after-free).

> Would it be possible
> to extend the shared list.h helpers (might this contain functionality
> interesting to other drivers as well)?

Hm, not without making them more complex I'm afraid. And since
most users are happy with the current implementation I'm not entirely
sure there's a need for such a feature.

> 
> What about the new next_ins argument -- should that always be required
> to specify?

AFAICT, it's only really needed in midgard_pair_load_store() to update
next_ins if the instruction we removed is next_ins. This being said, a
few functions were calling mir_next_op() manually and having next_ins
directly accessible saves us this extra call.

> Maybe we want a dedicated variant _with_next() to save the
> empty argument in the usual case (when we don't actually need it).

We can, but I also find it useful to expose this implementation detail
to users, so that they have a rough idea of what's happening behind the
scene (temporary storage pointing to the next instruction to allow
safe removal of the current item).