From 6e96c0df9702776a5ed8d43e852dcdd85ad47368 Mon Sep 17 00:00:00 2001 From: Connor Abbott Date: Tue, 6 Feb 2024 14:14:04 -0500 Subject: [PATCH] ir3/ra: Fix bug with collect source handling It can be the case that a collect and one of its sources are assigned to non-overlapping parts of the same merge set, for example: ssa_1 = ... ssa_2 = ... ssa_3 = ... ssa_4 = collect ssa_1, ssa_2 (kill), ssa_3 ... = ssa_4 (kill) ssa_5 = collect ssa_1, ssa_3 ... = ssa_1 (kill) ... = ssa_3 (kill) ... = ssa_5 (kill) If we merge the first collect first, we get a merge set: ssa_1 (offset 0) ssa_2 (offset 2) ssa_3 (offset 4) ssa_4 (offset 0) Now, we decide to merge ssa_1 and ssa_5: ssa_1 (offset 0) ssa_2 (offset 2) ssa_3 (offset 4) ssa_4 (offset 0) ssa_5 (offset 0) ssa_3 cannot become a child of ssa_5 in the interval tree, just like a source not in the same merge set, so we should not remove it and then reinsert it assuming that RA will make it a child of ssa_5. This fixes an RA validation error in Farming Simulater. Fixes: 0ffcb19 ("ir3: Rewrite register allocation") Part-of: (cherry picked from commit aeed5fd98d47be122260bf0c232c2d8dfcd8d400) --- .pick_status.json | 2 +- src/freedreno/ir3/ir3_ra.c | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 9083c5150ee..4aff5a711c6 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -2894,7 +2894,7 @@ "description": "ir3/ra: Fix bug with collect source handling", "nominated": true, "nomination_type": 1, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "0ffcb19b9d9fbe902224542047c389a661fbf816", "notes": null diff --git a/src/freedreno/ir3/ir3_ra.c b/src/freedreno/ir3/ir3_ra.c index 070f67ba7ba..bcd8a527fd3 100644 --- a/src/freedreno/ir3/ir3_ra.c +++ b/src/freedreno/ir3/ir3_ra.c @@ -1715,8 +1715,14 @@ handle_collect(struct ra_ctx *ctx, struct ir3_instruction *instr) struct ra_interval *interval = &ctx->intervals[src->def->name]; - if (src->def->merge_set != dst_set || interval->is_killed) + /* We only need special handling if the source's interval overlaps with + * the destination's interval. + */ + if (src->def->interval_start >= instr->dsts[0]->interval_end || + instr->dsts[0]->interval_start >= src->def->interval_end || + interval->is_killed) continue; + while (interval->interval.parent != NULL) { interval = ir3_reg_interval_to_ra_interval(interval->interval.parent); }