From 955a82bb83dafe51d46eee56a178142e59bfb39d Mon Sep 17 00:00:00 2001 From: Christoph Pillmayer Date: Mon, 2 Mar 2026 16:27:14 +0100 Subject: [PATCH] pan/bi: Fix coupling spill placement In the following arrangement the old logic leads to the following: | v +----------+------------+ |block5 | |m815 = PHI m1034, m860 |<-----------+ |343 = FMA.f32 ... | | +----------+------------+ | | | +--------------+ | | | | v v | +-----+ +-----+ | |b6 | |b7,8 | | | | | | | +-----+ +--+--+ | | +---+ | +---+ | +----|b9 +-----+----|b10+---+ | v +---+ +---+ v | +-------+-------------+ +-------+---------+ | |block12 | |block11 | | |m882 = PHI m815, m860| |m860 = MEMMOV 343+--+ +---------+-----------+ +-----------------+ v The spill of / into m860 (corresponding to 343) ends up in block11 when insert_coupling_code(succ=block5, pred=block11) because of the memory phi in block5. Later, in insert_coupling_code(block12, block9), we reject inserting the spill after ca9c9957. As a result, m860 is undefined along block5 -> block7,8 -> block9 -> block12. When the spill position is chosen first, ctx->block is block5 so choose_spill_position falsely returns the fallback position. The issue can be fixed by explicitly passing the "current block". Fixes: ca9c9957 ("pan: Avoid some redundant SSA spills") Reviewed-by: Eric R. Smith (cherry picked from commit 09e1ba28e5c697cc18075334a053bf9af65e7046) Part-of: --- .pick_status.json | 2 +- src/panfrost/compiler/bifrost/bi_spill_ssa.c | 27 +++++++++++++++----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 7bf7cf5b84f..7fb20e51c1c 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -3564,7 +3564,7 @@ "description": "pan/bi: Fix coupling spill placement", "nominated": true, "nomination_type": 2, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "ca9c9957e20bd33f9a79ca71ef7d72d8aef0d6e8", "notes": null diff --git a/src/panfrost/compiler/bifrost/bi_spill_ssa.c b/src/panfrost/compiler/bifrost/bi_spill_ssa.c index 9316ed09f6b..cb4d5173883 100644 --- a/src/panfrost/compiler/bifrost/bi_spill_ssa.c +++ b/src/panfrost/compiler/bifrost/bi_spill_ssa.c @@ -383,8 +383,21 @@ remat_to(bi_builder *b, bi_index dst, struct spill_ctx *ctx, unsigned node) } } +/* This tries to return a cursor in the block in which 'node' is defined. + * Returns the fallback position if the block that defines 'node' equals + * 'current_block'. + * + * 'current_block' if called from: + * - min_algorithm: ctx->block + * - insert_coupling_code: current_block should be the block in which the spill + * is needed (i.e. not ctx->block, if ctx->block == succ, but the spill is + * required from pred). The reason for this is, if a value defined in succ + * needs to be spilled first along the backedge back to succ from pred, the + * spilled version can be undefined at the place of a later use. + */ static bi_cursor -choose_spill_position(struct spill_ctx *ctx, unsigned node, bi_cursor fallback) +choose_spill_position(struct spill_ctx *ctx, unsigned node, bi_cursor fallback, + bi_block *current_block) { bi_instr *producer = ctx->ssa_defs[node]; bi_block *block = ctx->ssa_def_blocks[node]; @@ -392,7 +405,7 @@ choose_spill_position(struct spill_ctx *ctx, unsigned node, bi_cursor fallback) assert(producer); assert(block); - if (ctx->block == block) + if (current_block == block) return fallback; /* Don't insert spills in the middle of the PHI block. This breaks the @@ -569,8 +582,8 @@ insert_coupling_code(struct spill_ctx *ctx, bi_block *pred, bi_block *succ) } if (!spilled) { - bi_cursor c = choose_spill_position(ctx, I->src[s].value, - bi_after_block_logical(pred)); + bi_cursor c = choose_spill_position( + ctx, I->src[s].value, bi_after_block_logical(pred), pred); bi_builder b = bi_init_builder(ctx->shader, c); insert_spill(&b, ctx, I->src[s].value); @@ -615,7 +628,8 @@ insert_coupling_code(struct spill_ctx *ctx, bi_block *pred, bi_block *succ) if (spilled) continue; - bi_cursor c = choose_spill_position(ctx, v, bi_along_edge(pred, succ)); + bi_cursor c = choose_spill_position(ctx, v, bi_along_edge(pred, succ), + bi_edge_to_block(pred, succ)); bi_builder b = bi_init_builder(ctx->shader, c); insert_spill(&b, ctx, v); } @@ -1187,7 +1201,8 @@ limit(struct spill_ctx *ctx, bi_instr *I, unsigned m) * another use */ if (!BITSET_TEST(ctx->S, v) && candidates[i].dist < DIST_INFINITY) { - bi_cursor c = choose_spill_position(ctx, v, bi_before_instr(I)); + bi_cursor c = + choose_spill_position(ctx, v, bi_before_instr(I), ctx->block); bi_builder b = bi_init_builder(ctx->shader, c); insert_spill(&b, ctx, v); BITSET_SET(ctx->S, v);