[LiveVariables] Keep prior partial defs live across the closing partial def#204963
[LiveVariables] Keep prior partial defs live across the closing partial def#204963ravn wants to merge 1 commit into
Conversation
…al def When a use of a super-register is fed by independent writes to its sub-registers, LiveVariables routes the use through the LAST partial def by adding an `implicit-def Reg` to it. Commit e8e3e6e (llvm#119446) was meant to fix a separate issue where that implicit operand iteration sometimes added `implicit $sub` for a sub-register the same instruction already defines. The merged commit, however, removed the iteration entirely instead of guarding it. The unintended consequence: the earlier writers of OTHER sub-registers appear dead. DeadMachineInstrElim deletes them, and downstream code reads a now-undefined register. This is the AVR / Z80 / MSP430 miscompile from llvm#156428: a pair-half assignment vanishes before the super-reg is consumed by a call. Restore the iteration but guard it correctly: - Snapshot which sub-registers of Reg are already covered by an existing explicit def of LastPartialDef BEFORE adding the implicit-def Reg (modifiesRegister would otherwise mistakenly include Reg as covering everything). - For each remaining sub-register that has a prior partial def in this block, add an implicit-use to LastPartialDef. Skip sub-registers with no prior def — adding an implicit-use there would extend a live range into a region with no reaching def and crash the verifier. New test llvm/test/CodeGen/AVR/livevars-implicitdef.mir covers three shapes: the bug's reduction, the same pattern with an unrelated instruction between the two partial defs, and the partial-def-of-subreg case the suspect commit was originally trying to handle. Fixes llvm#156428. Assisted-by: Claude (Anthropic, claude-opus-4-7)
|
Hello @ravn 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-llvm-regalloc Author: Thorbjørn Ravn Andersen (ravn) ChangesFixes #156428. SummaryOn targets with sub-register pairs like AVR (and downstream Z80 work) This PR restores a critical piece of bookkeeping in
I have done this using Claude, but have tried to do this as clear and The text below explains things in more detail as written by Claude. I /Thorbjørn > Attribution boundary. Everything below this line was drafted by Detailed analysis (by Claude
|
|
I have read the policies and conform as well as I can. |
1 participant
Fixes #156428.
Summary
On targets with sub-register pairs like AVR (and downstream Z80 work) byte-sized arithmetic typically writes the two halves of a 16-bit register pair from independent instructions before passing the 16-bit register pair to a function. For Z80, the D and E halves of the DE pair are written by separate instructions before DE is passed as an argument.
This PR restores a critical piece of bookkeeping in
LiveVariables::HandlePhysRegUseallowingLiveVariablesto handle this correctly, plus a safety check that prevents extending a live range into a region with no reaching definition.+55 / -0inLiveVariables.cpp— most of which is explanatory comments; the actual logic change is about a dozen lines — plus a new MIR lit test on AVR with three cases: the bug's reduction, the same shape with an intervening unrelated instruction, and the partial-def-of-subreg case the 2026 commit was originally handling.I have done this using Claude, but have tried to do this as clear and simple from the issue as I could.
The text below explains things in more detail as written by Claude. I have reviewed it but not rewritten it, as it was clear enough on its own.
/Thorbjørn
Detailed analysis (by Claude
claude-opus-4-7, reviewed by Thorbjørn)The bug
LiveVariables::HandlePhysRegUsehandles a use of a super-register whose value was assembled by independent writes to its sub-registers — the canonical pattern on small-int targets where an ABI parameter pair (e.g. AVR's$r25r24) is composed by two independent 8-bit writes before being passed to a function.When the super-register has no direct prior def, the pass routes the use through the last partial def by adding
implicit-def Regto it. That alone is fine.What's missing in current
main: the earlier partial def of the other sub-register has no remaining user. Concretely, givenLiveVariables tags the LDIRdK with
implicit-def $r25r24. The first MOVRdRr (which wrote$r25) is now seen byDeadMachineInstrElimas unused —$r25is "redefined" before any consumer — and gets deleted. The downstream code reads an undefined$r25. Miscompile.Where it came from
git bisect(single commit) points toe8e3e6e893a2c944c8ce1878f290aa62843323e0(#119446). That PR was fixing a separate bug — the same iteration could add animplicit $subfor a sub-register that the same instruction already defines, which is invalid (you can't read a register that's defined on the same instruction unless it's a tied operand).The submitted fix in PR #119446 added an
IsDefinedHereguard that converted the would-beimplicit-useinto animplicit-deffor overlapping cases. The merged version reworked it differently — by removing the iteration entirely. That over-correction silently deleted the live-keeping bookkeeping for the case in this issue.The fix
HandlePhysRegUse, after the existingimplicit-def Regline, gains ~30 lines of restored bookkeeping with two guards.Snapshot which sub-registers are already covered by LastPartialDef's existing explicit defs, before mutating the operand list with
implicit-def Reg. This is necessary becausemodifiesRegister(Sub, TRI)looks at all defs including the just-added one, which would falsely report every sub-register as "covered" and skip the entire iteration.Add
implicit-usefor each remaining sub-register that has a prior partial def in this block. This is what keeps the earlier writer alive against DeadMachineInstrElim.Skip sub-registers with no prior def. Without this guard, adding an
implicit-usefor an undefined sub-register extends a live range into a region with no reaching def, which crashesLiveIntervals::computeRegUnitRangein MachineScheduler withLLVM ERROR: Invalid global physical register/Use not jointly dominated by defs. (Found by regression testing against three X86 tests on the first pass; this guard was the minimal fix.)The 2026 PR's
IsDefinedHere-becomes-implicit-defwidening for partially-overlapping sub-registers (e.g. promotingimplicit $sgpr2_sgpr3toimplicit-def $sgpr2_sgpr3when $sgpr3 is explicitly defined) is deliberately not part of this patch. The super-register's existingimplicit-def Regalready covers those bits for live-tracking purposes; the operand list stays shorter and the soundness story for this PR stays narrowly focused on the deletion bug.Test
llvm/test/CodeGen/AVR/livevars-implicitdef.miradds three cases:pair_two_halves_eqv_dist— the bug's reduction. Two consecutive independent writes followed by a super-reg use.pair_two_halves_hole— same shape with an unrelated instruction between the two partial defs (so LastPartialDef is not immediately adjacent to the earlier partial def).pair_partial_def_subreg_of_use— both halves written byMOVinstructions, exercises the case PR [LiveVariables] Mark use without implicit if defined at instr #119446 was originally trying to make sound. Confirms my fix doesn't reintroduce theimplicit $sub-where-sub-is-definedshape (it doesn't, because theAlreadyDefinedsnapshot viamodifiesRegistercatches the overlap and skips).Each positive case fails on stock
mainand passes with the patch.Regression sweep
Across
Transforms/CodeGen/{AVR,AArch64,X86}after building all required tools:Zero failures, zero regressions.
Note for reviewers
Two earlier attempts at the iteration's guard surfaced real test failures and were discarded:
implicit-usefor every sibling sub-register — causedInvalid global physical registerinMachineScheduleron the three X86 tests that hitLiveIntervals::computeRegUnitRange. ThePhysRegDef[SubReg]guard fixed those.IsDefinedHereviaisSubRegister(SubReg, MO.getReg())which only catches one direction of the sub-register relationship. Replaced withmodifiesRegister, which handles both subset and superset and is the standard idiom elsewhere in this file.The included AVR test is the third case from the bisect of those failures; it exercises both guard conditions in concert.