From c9bebae2877e55cdcd94f9f9f3f6805238caeb28 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Tue, 27 Aug 2019 12:36:43 +0200 Subject: [PATCH] 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 Reviewed-by: Alyssa Rosenzweig --- src/panfrost/midgard/midgard_schedule.c | 63 ++++++++++++------------- 1 file changed, 29 insertions(+), 34 deletions(-) diff --git a/src/panfrost/midgard/midgard_schedule.c b/src/panfrost/midgard/midgard_schedule.c index b8d9b5ec9be..feb988a66ba 100644 --- 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; } } -- 2.30.2