From 05f6b63910dce8e942c43b0b4b389fc37d677c58 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Fri, 11 Apr 2025 12:30:43 -0700 Subject: [PATCH] Fix minimal-bundle splitting to avoid infinite loops. This PR reworks minimal-bundle logic to be consistent between property computation and splitting, and overall simpler. To recap: a "minimal bundle" is an allocation bundle that is as small as possible. Because RA2 does backtracking, i.e., can kick a bundle out of an allocation to make way for another, and because it can split, i.e. create more work for itself, we need to ensure the algorithm "bottoms out" somewhere to ensure termination. The way we do this is by defining a "minimal bundle": this is a bundle that cannot be split further. A minimal bundle is never evicted, and any allocatable input (set of uses/defs with constraints), when split into minimal bundles, should result in no conflicts. We thus want to define minimal bundles as *minimally* as possible so that we have the maximal solving capacity. For a long time, we defined minimal bundles as those spanning a single instruction (before- and after- progpoints) -- because we cannot insert moves in the middle of an instruction. In #214, we updated minimal-bundle splitting to avoid putting two different unrelated uses in a single-instruction bundle, beacuse doing so and calling it "minimal" (unsplittable) can artificially extend the liverange of a use with a fixed-reg constraint at the before-point into the after-point, causing an unsolveable conflict. This was triggered by new and tighter constraints on callsites in Cranelift after bytecodealliance/wasmtime#10502 (merging retval defs into calls) landed. Unfortunately this also resulted in an infinite allocation loop, because the definition of "minimal bundle" did not agree between the split-into-minimal-bundles fallback/last-ditch code, and the bundle property computation. The splitter was splitting as far as it was willing to go, but our predicate didn't consider those bundles minimal, so we continued to re-attempt splitting indefinitely. While investigating this, I found that the minimal-bundle concept had accumulated significant cruft ("the detritus of dead fuzzbugs") and this tech-debt was making things more confusing than not -- so I started by clearly defining what a minimal bundle *is*. Precisely: - A single use, within a single LiveRange; - With that LiveRange having a program-point span consistent with the use: - Early def: whole instruction (must live past Late point so it can reach its uses; moves not possible within inst); - Late def: Late point only; - Early use: Early point only; - Late use: whole instruction (must be live starting at Early so the value can reach this use; moves not possible within inst). This is made easier and simpler than what we have before largely because the minimal-bundle splitter aggressively puts spans of LiveRange without uses into the spill bundle, and because we support overlapping LiveRanges for a vreg now (thanks Trevor!), so we can rely on having *some* connectivity between the def and its uses even if we aggressively trim LiveRanges in the minimal bundles down to just their defs/uses. The split-at-program-point splitter (i.e., not the fallback split-into-minimal-bundles splitter) also got a small fix related to this: it has a mode that was intended to "split off one use" if we enter with a split-point at the start of the bundle, but this was really splitting off all uses at the program point (if there are multiple of the same vreg at the same program point). In the case that we still need to split these apart, this just falls back to the minimal-bundle splitter now. Fixes #218, #219. --- src/ion/mod.rs | 3 + src/ion/process.rs | 163 ++++++++++++++++++++++----------------------- 2 files changed, 82 insertions(+), 84 deletions(-) diff --git a/src/ion/mod.rs b/src/ion/mod.rs index 0de5f87a..51751d5e 100644 --- a/src/ion/mod.rs +++ b/src/ion/mod.rs @@ -97,6 +97,9 @@ impl<'a, F: Function> Env<'a, F> { self.process_bundles()?; self.try_allocating_regs_for_spilled_bundles(); self.allocate_spillslots(); + if trace_enabled!() { + self.dump_state(); + } let moves = self.apply_allocations_and_insert_moves(); Ok(self.resolve_inserted_moves(moves)) } diff --git a/src/ion/process.rs b/src/ion/process.rs index 75abadd9..13f04e8b 100644 --- a/src/ion/process.rs +++ b/src/ion/process.rs @@ -19,7 +19,7 @@ use super::{ }; use crate::{ ion::data_structures::{ - CodeRange, BUNDLE_MAX_NORMAL_SPILL_WEIGHT, MAX_SPLITS_PER_SPILLSET, + CodeRange, Use, BUNDLE_MAX_NORMAL_SPILL_WEIGHT, MAX_SPLITS_PER_SPILLSET, MINIMAL_BUNDLE_SPILL_WEIGHT, MINIMAL_FIXED_BUNDLE_SPILL_WEIGHT, }, Allocation, Function, Inst, InstPosition, OperandConstraint, OperandKind, PReg, ProgPoint, @@ -292,15 +292,23 @@ impl<'a, F: Function> Env<'a, F> { break; } } - // Minimal if there is only one LR and the ProgPoint range - // covers only one instruction. Note that it could cover - // just one ProgPoint, - // i.e. X.Before..X.After, or two ProgPoints, - // i.e. X.Before..X+1.Before. - trace!(" -> first range has range {:?}", first_range_data.range); - let bundle_start = self.ctx.bundles[bundle].ranges.first().unwrap().range.from; - let bundle_end = self.ctx.bundles[bundle].ranges.last().unwrap().range.to; - minimal = bundle_start.inst() == bundle_end.prev().inst(); + // Minimal if there is only one LR and only up to one use + // in that LR, and extent of range is consistent with the + // "minimal range for use" definition. + minimal = match &first_range_data.uses[..] { + [] => true, + [only_use] => { + // We use a "contains" rather than "exact" range + // check because a smaller-than-min range is also + // OK, if some logic produced it for a valid + // reason -- for example, a dead def. We don't + // want to livelock on "smaller than minimal" + // ranges. + let min_range = minimal_range_for_use(only_use); + min_range.contains(&first_range_data.range) + } + _ => false, + }; trace!(" -> minimal: {}", minimal); } else { minimal = false; @@ -430,9 +438,26 @@ impl<'a, F: Function> Env<'a, F> { let bundle_end = self.ctx.bundles[bundle].ranges.last().unwrap().range.to; debug_assert!(split_at < bundle_end); + trace!( + "bundle_start = {:?} bundle_end = {:?}", + bundle_start, + bundle_end + ); + + // Is the bundle at most spanning one instruction? Then + // there's nothing we can do with this logic (we will not + // split in the middle of the instruction). Fall back to the + // minimal-bundle splitting in this case as well. + if bundle_end.prev().inst() == bundle_start.inst() { + trace!(" -> spans only one inst; splitting into minimal bundles"); + self.split_into_minimal_bundles(bundle, reg_hint); + return; + } + // Is the split point *at* the start? If so, peel off the - // first use: set the split point just after it, or just - // before it if it comes after the start of the bundle. + // program point with the first use: set the split point just + // after it, or just before it if it comes after the start of + // the bundle. if split_at == bundle_start { // Find any uses; if none, just chop off one instruction. let mut first_use = None; @@ -762,7 +787,7 @@ impl<'a, F: Function> Env<'a, F> { /// /// This is meant to solve a quadratic-cost problem that exists /// with "normal" splitting as implemented above. With that - /// procedure, , splitting a bundle produces two + /// procedure, splitting a bundle produces two /// halves. Furthermore, it has cost linear in the length of the /// bundle, because the resulting half-bundles have their /// requirements recomputed with a new scan, and because we copy @@ -804,17 +829,10 @@ impl<'a, F: Function> Env<'a, F> { reg_hint ); - let mut last_lr: Option = None; - let mut last_bundle: Option = None; - let mut last_inst: Option = None; - let mut last_vreg: Option = None; - let mut spill_uses = UseList::new_in(self.ctx.bump()); let empty_vec = LiveRangeList::new_in(self.ctx.bump()); for entry in core::mem::replace(&mut self.ctx.bundles[bundle].ranges, empty_vec) { - let lr_from = entry.range.from; - let lr_to = entry.range.to; let vreg = self.ctx.ranges[entry.index].vreg; self.ctx.scratch_removed_lrs.insert(entry.index); @@ -825,10 +843,9 @@ impl<'a, F: Function> Env<'a, F> { let mut spill_range = entry.range; let mut spill_starts_def = false; - let mut last_live_pos = entry.range.from; let empty_vec = UseList::new_in(self.ctx.bump()); for u in core::mem::replace(&mut self.ctx.ranges[entry.index].uses, empty_vec) { - trace!(" -> use {:?} (last_live_pos {:?})", u, last_live_pos); + trace!(" -> use {:?}", u); let is_def = u.operand.kind() == OperandKind::Def; @@ -848,66 +865,17 @@ impl<'a, F: Function> Env<'a, F> { continue; } - // If this is a def of the vreg the entry cares about, make sure that the spill - // range starts right before the next instruction so that the value is available. + // If this is a def, make sure that the spill range + // starts before the next instruction rather than at + // this position: the value is available only *after* + // this position. if is_def { trace!(" -> moving the spill range forward by one"); spill_range.from = ProgPoint::before(u.pos.inst().next()); } - // If we just created a LR for this inst at the last - // pos, add this use to the same LR. - if Some(u.pos.inst()) == last_inst && Some(vreg) == last_vreg { - self.ctx.ranges[last_lr.unwrap()].uses.push(u); - trace!(" -> appended to last LR {:?}", last_lr.unwrap()); - continue; - } - - // The minimal bundle runs through the whole inst - // (up to the Before of the next inst), *unless* - // the original LR was only over the Before (up to - // the After) of this inst. - let to = core::cmp::min(ProgPoint::before(u.pos.inst().next()), lr_to); - - // If the last bundle was at the same inst, add a new - // LR to the same bundle; otherwise, create a LR and a - // new bundle. - if Some(u.pos.inst()) == last_inst { - let cr = CodeRange { from: u.pos, to }; - let lr = self.ctx.ranges.add(cr, self.ctx.bump()); - new_lrs.push((vreg, lr)); - self.ctx.ranges[lr].uses.push(u); - self.ctx.ranges[lr].vreg = vreg; - - trace!( - " -> created new LR {:?} but adding to existing bundle {:?}", - lr, - last_bundle.unwrap() - ); - // Edit the previous LR to end mid-inst. - self.ctx.bundles[last_bundle.unwrap()] - .ranges - .last_mut() - .unwrap() - .range - .to = u.pos; - self.ctx.ranges[last_lr.unwrap()].range.to = u.pos; - // Add this LR to the bundle. - self.ctx.bundles[last_bundle.unwrap()] - .ranges - .push(LiveRangeListEntry { - range: cr, - index: lr, - }); - self.ctx.ranges[lr].bundle = last_bundle.unwrap(); - last_live_pos = ProgPoint::before(u.pos.inst().next()); - continue; - } - - // Otherwise, create a new LR. - let pos = ProgPoint::before(u.pos.inst()); - let pos = core::cmp::max(lr_from, pos); - let cr = CodeRange { from: pos, to }; + // Create a new LR. + let cr = minimal_range_for_use(&u); let lr = self.ctx.ranges.add(cr, self.ctx.bump()); new_lrs.push((vreg, lr)); self.ctx.ranges[lr].uses.push(u); @@ -936,13 +904,6 @@ impl<'a, F: Function> Env<'a, F> { cr, new_bundle ); - - last_live_pos = ProgPoint::before(u.pos.inst().next()); - - last_lr = Some(lr); - last_bundle = Some(new_bundle); - last_inst = Some(u.pos.inst()); - last_vreg = Some(vreg); } if !spill_range.is_empty() { @@ -1363,3 +1324,37 @@ impl<'a, F: Function> Env<'a, F> { return Ok(()); } } + +/// Compute the minimal range that covers a given use in a minimal +/// bundle. This definition needs to be consistent between bundle +/// property computation and minimal-range splitting (fallback +/// splitting). +/// +/// The cases are: +/// - early def: whole instruction +/// - late def: Late only +/// - early use: Early only +/// - late use: whole instruction +fn minimal_range_for_use(u: &Use) -> CodeRange { + let early = ProgPoint::before(u.pos.inst()); + let late = ProgPoint::after(u.pos.inst()); + let next_early = ProgPoint::before(u.pos.inst().next()); + match (u.pos.pos(), u.operand.kind()) { + (InstPosition::Before, OperandKind::Def) => CodeRange { + from: early, + to: next_early, + }, + (InstPosition::Before, OperandKind::Use) => CodeRange { + from: early, + to: late, + }, + (InstPosition::After, OperandKind::Def) => CodeRange { + from: late, + to: next_early, + }, + (InstPosition::After, OperandKind::Use) => CodeRange { + from: early, + to: next_early, + }, + } +}