panfrost: Rework midgard_pair_load_store() to kill the nested foreach loop

mir_foreach_instr_in_block_safe() is based on list_for_each_entry_safe()
which is designed to protect against removal of the current entry, but
removing the entry placed just after the current one will lead to a
use-after-free situation.

Luckily, the midgard_pair_load_store() logic guarantees that the
instruction being removed (if any) is never placed just after ins which
in turn guarantees that the hidden __next variable always points to a
valid object.
Took me a bit of time to realize that this code was safe, so I'm
suggesting to get rid of the inner mir_foreach_instr_in_block_from()
loop and rework the code so that the removed instruction is always the
current one (which is what the list_for_each_entry_safe() API was
initially designed for).

While at it, we also get rid of the unecessary insert(ins)/remove(ins)
dance by simply moving the instruction around.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

diff --git a/src/panfrost/midgard/midgard_schedule.c b/src/panfrost/midgard/midgard_schedule.c
index b8d9b5ec9be1538cf919e4f1e191c79bae38dc4d..feb988a66badb46eea46f7e5bf3d0176ff0ca789 100644 (file)
--- a/src/panfrost/midgard/midgard_schedule.c
+++ b/src/panfrost/midgard/midgard_schedule.c
@@ -657,46 +657,41 @@ 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) {
-                if (ins->type != TAG_LOAD_STORE_4) continue;
-
-                /* We've found a load/store op. Check if next is also load/store. */
-                midgard_instruction *next_op = mir_next_op(ins);
-                if (&next_op->link != &block->instructions) {
-                        if (next_op->type == TAG_LOAD_STORE_4) {
-                                /* If so, we're done since we're a pair */
-                                ins = mir_next_op(ins);
-                                continue;
-                        }
-
-                        /* Maximum search distance to pair, to avoid register pressure disasters */
-                        int search_distance = 8;
-
-                        /* Otherwise, we have an orphaned load/store -- search for another load */
-                        mir_foreach_instr_in_block_from(block, c, mir_next_op(ins)) {
-                                /* Terminate search if necessary */
-                                if (!(search_distance--)) break;
-
-                                if (c->type != TAG_LOAD_STORE_4) continue;
-
-                                /* We can only reorder if there are no sources */
-
-                                bool deps = false;
+        midgard_instruction *prev_ldst = NULL;
+        int search_distance;
 
-                                for (unsigned s = 0; s < ARRAY_SIZE(ins->src); ++s)
-                                        deps |= (c->src[s] != ~0);
+        mir_foreach_instr_in_block_safe(block, ins) {
+                if (ins->type != TAG_LOAD_STORE_4 && !prev_ldst) continue;
 
-                                if (deps)
-                                        continue;
+                /* We've found a load/store op. Start searching for another one.
+                 * Maximum search distance to pair, to avoid register pressure disasters
+                 */
+                if (!prev_ldst) {
+                        search_distance = 8;
+                        prev_ldst = ins;
+                        continue;
+                }
 
-                                /* We found one! Move it up to pair and remove it from the old location */
+                /* Already paired. */
+                if (mir_prev_op(ins) == prev_ldst) {
+                        prev_ldst = NULL;
+                        continue;
+                }
 
-                                mir_insert_instruction_before(ctx, ins, *c);
-                                mir_remove_instruction(c);
+                /* We can only reorder if there are no sources */
+                bool deps = false;
+                for (unsigned s = 0; s < ARRAY_SIZE(ins->src); ++s)
+                        deps |= (ins->src[s] != ~0);
 
-                                break;
-                        }
+                /* We found one! Move it up to pair */
+                if (!deps) {
+                        list_del(&ins->link);
+                        list_add(&ins->link, &prev_ldst->link);
+                        continue;
                 }
+
+                if (!(search_distance--))
+                        prev_ldst = NULL;
         }
 }