Repository navigation
ion: preserve register limits through reuse and fixups - #267
rahulchaphalkar wants to merge 1 commit into
Conversation
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks. However I think this needs a lot more explanation, and/or exploration. Rather than write a test for one case, we need to update the fuzzer to generate the relevant constraint cases, and then we need to fuzz extensively before merging; otherwise, seemingly innocent point-fixes like this can often break other things (as I worry might be the case here -- see below).
For the specific problem of Reuse constraints that refer to other Limit-constrained vregs: we generally don't propagate constraints on a Reuse otherwise and things work fine -- I think we should be able to either rely on the usual constraint meet-pass on actual bundle merge, if the two halves are merged, or if not, we should change the way that the fixup-move emission + preprocessing logic on reuse constraints works. The fix here is brittle and I worry incorrect in some cases.
| } | ||
|
|
||
| #[test] | ||
| fn ion_limited_operand_with_multiple_fixed_uses() { |
There was a problem hiding this comment.
Why is the test in fastalloc named with ion?
| insts: vec![vec![op(Def, 0, FixedReg(p(31)))], uses.clone()], | ||
| }]); | ||
| let options = RegallocOptions { | ||
| algorithm: Algorithm::Ion, |
There was a problem hiding this comment.
Ah, I see -- you put a test for Ion in fastalloc. Although it seems weird, the Ion backend actually doesn't have unit tests -- they're verbose and hard to write and maintain, and instead, we update the fuzzer to generate the cases we want to accept (and the crate is continuously fuzzed so this will quickly find issues).
Rather than add this test, can you update the fuzzing function generator to emit functions that hit the newly problematic cases?
| } | ||
| OperandConstraint::Limit(max) => { | ||
| first_reg_slot.get_or_insert(u.slot); | ||
| first_reg_slot = Some(u.slot); |
There was a problem hiding this comment.
Can you explain (in a comment ideally) why we overwrite first_reg_slot each time we see a Limit, rather than save the first one?
If there is some lattice meet semantics we want here, e.g. Limit overrides Reg, we should do that explicitly rather than rely on visit order, which can be brittle...
| for u in &self.ranges[entry.index].uses { | ||
| use OperandConstraint::*; | ||
| match u.operand.constraint() { | ||
| match self.operand_constraint(u.operand, u.pos.inst()) { |
There was a problem hiding this comment.
I'm not sure I understand why we need to "look through" Reuse constraints here -- we don't do that anywhere else, and it's likely to break things wrt minimal bundles, I suspect.
Why can't we propagate any constraints after we merge, if we merge?
2 participants
Fix two Ion correctness issues discovered while testing register constraints for Cranelift’s APX/extended-register support. Both affect generic OperandConstraint::Limit handling.
Preserve limits through reuse. When an output reuses a limited input, propagate that input’s constraint into requirement and bundle calculations.
E.g. Previously, Ion could assign p30 to an input constrained by Limit(16).
Keep multi-fixed fixup sources stable. When a value has both limited and fixed uses, select the limited operand as the fixup source. Otherwise, another fixup can overwrite the selected source allocation, causing subsequent moves to read the wrong location.