Skip to content

Conversation

@abrown
Copy link
Member

@abrown abrown commented Dec 13, 2025

Consider an instruction that could potentially overallocate to a limited range of registers:

Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)

Both uses of v0 must fit in registers 0-1 and v1 should find its home somewhere else in registers 2-7. But fuzzing found a case where this failed. If v1 was allocated first--say to register 0--and one of the uses of v0 allocated to the remaining register--register 1--the final use of v0 had nowhere to go and could not evict either of the previous allocations. This caused ion to fail with TooManyLiveRegs when in fact a solution was possible.

(Why are the identical uses of v0 allocated separately? Can't they use the same register? I'm not entirely sure, but I think ion had split things down to minimal bundles).

The fix in this change is to alter the default spill weights assigned to a minimal bundle. Previously, minimal bundles with a fixed constraint received the highest weight, followed by other minimal bundles and then non-minimal bundles. This reserves weights for limits into that order:

  • minimal with fixed constraint
  • minimal with limit(0..=0) constraint
  • minimal with limit(0..=1) constraint
  • ...
  • minimal with limit(0..=255) constraint
  • minimal, any other constraint
  • non-minimal

Doing this allows ion to evict bundles with higher limits (e.g., v1i limit(0..=7) once they become minimal, allowing allocation to continue.

Consider an instruction that could potentially overallocate to a limited
range of registers:

```
Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)
```

Both uses of `v0` _must_ fit in registers 0-1 and `v1` should find its
home somewhere else in registers 2-7. But fuzzing found a case where
this failed. If `v1` was allocated first--say to register 0--and one of
the uses of `v0` allocated to the remaining register--register 1--the
final use of `v0` had nowhere to go _and_ could not evict either of the
previous allocations. This caused `ion` to fail with `TooManyLiveRegs`
when in fact a solution was possible.

(Why are the identical uses of `v0` allocated separately? Can't they use
the same register? I'm not entirely sure, but I think `ion` had split
things down to minimal bundles).

The fix in this change is to alter the default spill weights assigned to
a minimal bundle. Previously, minimal bundles with a fixed constraint
received the highest weight, followed by other minimal bundles and then
non-minimal bundles. This reserves weights for limits into that order:
- minimal with `fixed` constraint
- minimal with `limit(0..=0)` constraint
- minimal with `limit(0..=1)` constraint
- ...
- minimal with `limit(0..=255)` constraint
- minimal, any other constraint
- non-minimal

Doing this allows `ion` to evict bundles with higher  limits (e.g., `v1i
limit(0..=7)` once they become minimal, allowing allocation to continue.

Co-authored-by: Chris Fallin <chris@cfallin.org>
Co-authored-by: Rahul Chaphalkar <rahul.s.chaphalkar@intel.com>
@abrown abrown marked this pull request as draft December 13, 2025 01:07
@abrown
Copy link
Member Author

abrown commented Dec 13, 2025

No hurry to merge this just yet; I'm running this in my branch that enables fuzzing of limits: fuzz-limit-constraints-rebased. Thanks to @rahulchaphalkar for helping discover a minimizable test case and @cfallin for talking through a solution!

@abrown
Copy link
Member Author

abrown commented Dec 13, 2025

Also, if it is helpful, I can include an extra commit here with a unit test that runs the exact test case that caused the original problem:

{
  block0(): # succs:[] preds:[]
    inst0: Op ops:[Def@Early: v0i limit(0..=3)] clobber:[]
    inst1: Op ops:[Def: v1i fixed(p25i), Use: v0i fixed(p26i), Use: v0i fixed(p27i)] clobber:[]
    inst2: Op ops:[Def: v2i reg, Use: v0i limit(0..=1), Use: v0i limit(0..=1), Use: v1i limit(0..=7)] clobber:[]
    inst3: Op ops:[Def: v3i reg, Use: v0i fixed(p28i), Use: v0i any] clobber:[]
    inst4: Ret ops:[] clobber:[]
}

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (no surprise since we pair-programmed this fix) -- thanks a bunch for working on this!

Regarding the unit test for this case -- I guess it depends on whether the fuzzer would cover this (ie the reason we don't have a test suite otherwise, since the fuzzer coverage is so good). This case was originally discovered via fuzzing and the original fuzzbug is now fixed? If so then I think it's fine; otherwise it might be good to throw in an explicit test. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants