Some fixes to allow for call instructions to name args, returns, and clobbers with constraints.#74
Conversation
|
This one is pretty subtle but perhaps one of @fitzgen, @elliottt, @jameysharp would be willing to review? I'm happy to do a live review / talk through it if that's easier. |
cfallin
force-pushed
the
overlapping-fixed-use-def
branch
from
20d6dc5 to
1c1ceb6
Compare
|
Updated to be a little more generic with respect to safepoint handling (not special-casing early defs) as per suggestion by @jameysharp in offline discussion just now. Thanks! Should be ready to review now. |
|
Another update pushed after finding a bug in the above changes while fuzzing locally. |
…clobbers with constraints. - Allow early-pos uses with fixed regs that conflict with clobbers (which happen at late-pos), in addition to the existing logic for conflicts with late-pos defs with fixed regs. This is a pretty subtle issue that was uncovered in bytecodealliance#53 for the def case, and the fix here is the mirror of that fix for clobbers. The root cause for all this complexity is that we can't split in the middle of an instruction (because there's no way to insert a move there!) so if a use is live-downward, we can't let it live in preg A at early-pos and preg B != A at late-pos; instead we need to rewrite the constraints and use a fixup move. The earlier change to fix bytecodealliance#53 was actually a bit too conservative in that it always applied when such conflicts existed, even if the downward arg was not live. This PR fixes that (it's fine for the early-use and late-def to be fixed to the same reg if the use's liverange ends after early-pos) and adapts the same flexibility to the clobbers case as well. - Reworks the fixups for the def case mentioned above to not shift the def to the Early point. Doing so causes issues when the def is to a reffy vreg: it can then be falsely included in a stackmap if the instruction containing this operand is a safepoint. - Fixes the last-resort split-bundle-into-minimal-pieces logic from bytecodealliance#59 to properly limit a minimal bundle piece to end after the early-pos, rather than cover the entire instruction. This was causing artificial overlaps between args that end after early-pos and defs that start at late-pos when one of the vregs hit the fallback split behavior.
This can create impossible situations: e.g., if a vreg is constrained to p0 as a late-def, and another, completely different vreg is constrained to p0 as an early-use on the same instruction, and the instruction also has a third vreg (early-use), we do not want to merge the liverange for the third vreg with the first, because it would result in an unsolveable conflict for p0 at the early-point.
cfallin
force-pushed
the
overlapping-fixed-use-def
branch
from
6061c98 to
98adab7
Compare
|
Rebased to current main. |
| self.bundles[bundle.index()].set_cached_fixed(); | ||
| } | ||
| if fixed_def { | ||
| self.bundles[bundle.index()].set_cached_fixed_def(); |
There was a problem hiding this comment.
Does this flag ever get cleared, or does it follow the parts of the bundle if it's split up?
There was a problem hiding this comment.
It's recomputed along with the other flags when the bundle is split, if it ever is (it actually is a flag on the whole bundle!).
There was a problem hiding this comment.
| if self.bundles[from.index()].cached_fixed_def() | ||
| || self.bundles[to.index()].cached_fixed_def() |
There was a problem hiding this comment.
If you knew that the fixed def constraints were for the same register, would that change whether or not you needed to bail out here?
There was a problem hiding this comment.
No, in fact the fuzzbug was exactly this case! The issue is that the way that we do the constraint rewrite adds an anonymous reservation; so this isn't a case of "different fixed def, can't merge" (that is captured by the incompatible-requirement check) but rather "corner case results in impossible problem".
The interesting case is when we have a fixed-use bound to preg P with liverange up to the early point on an inst, a fixed-def bound to preg P (same preg) at the late point on an inst, and that def'd vreg flows into a blockparam arg and over a backedge to the use's vreg. Then we try to merge the two vregs into one bundle (because we try this for every blockparam arg - blockparam def pair). But we rewrote the use to be an "any" use and put an anonymous reservation on preg P at the early-point only. So rejoining the first vreg (the use-side) to the whole bundle and thus requiring it to have the fixed-preg constraint again causes an impossible-to-solve problem.
There may be an even more complex way to rewrite this but I was several levels deep into "introduce fix, get deeper bug" and wanted to bail out of the rabbithole altogether...
There was a problem hiding this comment.
This check is very conservative and is hurting our compiler quite a bit since it makes heavy use of fixed operands. Removing this check gives a 4% code size reduction (I enabled the checker to ensure the result is still valid).
What are the exact requirements that cause the impossible to resolve constraint? Could this check be made more specific?
There was a problem hiding this comment.
I don't really have details paged in anymore, beyond the paragraph describing a fuzzing-discovered example I wrote above, nor the cycles at the moment to think through it deeply. You're welcome to remove it and fuzz to (re)discover it, and propose a better fix, of course!
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
This looks good to me! Thanks for answering my questions and for the thorough write-up :)
…f original LR. When a liverange starts at a *late* point of an instruction, and it undergoes the fallback "split into all minimal pieces" transform, we end up creating one minimal bundle that starts at the *early* point of the instruction at the start of the original LR. This can create impossible-to-allocate situations where a fixed-constraint LR overlaps another constrained to the same register (e.g. at calls). We fix this by ensuring the minimal bundle is trimmed only to the half of the instruction that overlaps the original LR. This is analogous to the third fix in bytecodealliance#74, but on the other end (start of LR rather than end of it).
…f original LR. (#85) When a liverange starts at a *late* point of an instruction, and it undergoes the fallback "split into all minimal pieces" transform, we end up creating one minimal bundle that starts at the *early* point of the instruction at the start of the original LR. This can create impossible-to-allocate situations where a fixed-constraint LR overlaps another constrained to the same register (e.g. at calls). We fix this by ensuring the minimal bundle is trimmed only to the half of the instruction that overlaps the original LR. This is analogous to the third fix in #74, but on the other end (start of LR rather than end of it).
3 participants
Allow early-pos uses with fixed regs that conflict with clobbers (which happen at late-pos), in addition to the existing logic for conflicts with late-pos defs with fixed regs.
This is a pretty subtle issue that was uncovered in Impossible constraints when a def and use have same fixed-reg constraint and use is live-out #53 for the def case, and the fix here is the mirror of that fix for clobbers. The root cause for all this complexity is that we can't split in the middle of an instruction (because there's no way to insert a move there!) so if a use is live-downward, we can't let it live in preg A at early-pos and preg B != A at late-pos; instead we need to rewrite the constraints and use a fixup move.
The earlier change to fix Impossible constraints when a def and use have same fixed-reg constraint and use is live-out #53 was actually a bit too conservative in that it always applied when such conflicts existed, even if the downward arg was not live. This PR fixes that (it's fine for the early-use and late-def to be fixed to the same reg if the use's liverange ends after early-pos) and adapts the same flexibility to the clobbers case as well.
Reworks the fixups for the def case mentioned above to not shift the def to the Early point. Doing so causes issues when the def is to a reffy vreg: it can then be falsely included in a stackmap if the instruction containing this operand is a safepoint.
Fixes the last-resort split-bundle-into-minimal-pieces logic from Limit split count per original bundle with fallback 1-to-N split. #59 to properly limit a minimal bundle piece to end after the early-pos, rather than cover the entire instruction. This was causing artificial overlaps between args that end after early-pos and defs that start at late-pos when one of the vregs hit the fallback split behavior.