Mesa (master): panfrost: Rework midgard_pair_load_store() to kill the nested foreach loop
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Fri Sep 13 10:16:42 UTC 2019
Module: Mesa
Branch: master
Commit: c9bebae2877e55cdcd94f9f9f3f6805238caeb28
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=c9bebae2877e55cdcd94f9f9f3f6805238caeb28
Author: Boris Brezillon <boris.brezillon at collabora.com>
Date: Tue Aug 27 12:36:43 2019 +0200
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 at collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com>
---
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;
}
}
More information about the mesa-commit
mailing list