From 49cb25a210309b8b90e0ad5333c5eef146969ee5 Mon Sep 17 00:00:00 2001 From: Benjamin Maxwell Date: Tue, 16 Sep 2025 13:57:41 +0000 Subject: [PATCH 1/4] Precommit test --- .../X86/rematerialize-sub-super-reg.mir | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir index b99c5fc8df0cb..13b90e715a7ad 100644 --- a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir +++ b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir @@ -165,5 +165,27 @@ body: | bb.3: $rax = COPY %t3 RET 0, $rax - ... +--- +; FIXME: `$al = COPY killed %4` should rematerialize as `dead $eax = MOV32r0 ... implicit-def $al` +; not `dead $eax = MOV32r0 ... implicit-def $rax` (as the full $rax is not used). +name: rematerialize_superregister_into_subregister_def_with_impdef_physreg +body: | + bb.0.entry: + ; CHECK-LABEL: name: rematerialize_superregister_into_subregister_def_with_impdef_physreg + ; CHECK: dead $esi = MOV32r0 implicit-def dead $eflags, implicit-def $rsi + ; CHECK-NEXT: dead $edx = MOV32r0 implicit-def dead $eflags, implicit-def $rdx + ; CHECK-NEXT: FAKE_USE implicit killed $rsi, implicit killed $rdx + ; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def $rax + ; CHECK-NEXT: FAKE_USE implicit killed $al + ; CHECK-NEXT: $eax = MOV32r0 implicit-def dead $eflags + ; CHECK-NEXT: RET 0, $eax + undef %1.sub_32bit:gr64_with_sub_8bit = MOV32r0 implicit-def dead $eflags, implicit-def %1 + $rsi = COPY %1 + $rdx = COPY %1 + FAKE_USE implicit killed $rsi, implicit killed $rdx + %4:gr8 = COPY killed %1.sub_8bit + $al = COPY killed %4 + FAKE_USE implicit killed $al + $eax = MOV32r0 implicit-def dead $eflags + RET 0, killed $eax From 11fbc6e2509e4af3e6de23cb7b3f2c5882caa999 Mon Sep 17 00:00:00 2001 From: Benjamin Maxwell Date: Tue, 16 Sep 2025 14:42:02 +0000 Subject: [PATCH 2/4] [RegisterCoalescer] Mark implicit-defs of super-registers as dead in remat Currently, something like: ``` $eax = MOV32ri -11, implicit-def $rax %al = COPY $eax ``` Can be rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def $rax ``` Which marks the full $rax as used, not just $al. With this change, this is rematerialized as: ``` dead $eax = MOV32ri -11, implicit-def dead $rax, implicit-def $al ``` To indicate that only $al is used. This issue is latent right now, but is exposed when #134408 is applied, as it results in the register pressure being incorrectly calculated. I think this change is in line with past fixes in this area, notably: https://github.com/llvm/llvm-project/commit/059cead5ed7aa11ce1eae0bcc751ea0d1e23ea75 https://github.com/llvm/llvm-project/commit/69cd121dd9945429b565b6a5eb8719130de880a7 --- llvm/lib/CodeGen/RegisterCoalescer.cpp | 30 +++++++++++++++++-- .../X86/rematerialize-sub-super-reg.mir | 4 +-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp index b8486f6560c5f..d117edb6838bd 100644 --- a/llvm/lib/CodeGen/RegisterCoalescer.cpp +++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp @@ -1475,7 +1475,8 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, // The implicit-def of the super register may have been reduced to // subregisters depending on the uses. - bool NewMIDefinesFullReg = false; + TinyPtrVector NewMIImpDefDestReg; + [[maybe_unused]] unsigned NewMIOpCount = NewMI.getNumOperands(); SmallVector NewMIImplDefs; for (unsigned i = NewMI.getDesc().getNumOperands(), @@ -1486,7 +1487,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, assert(MO.isImplicit()); if (MO.getReg().isPhysical()) { if (MO.getReg() == DstReg) - NewMIDefinesFullReg = true; + NewMIImpDefDestReg.push_back(&MO); assert(MO.isImplicit() && MO.getReg().isPhysical() && (MO.isDead() || @@ -1640,9 +1641,32 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, // been asked for. If so it must implicitly define the whole thing. assert(DstReg.isPhysical() && "Only expect virtual or physical registers in remat"); + + // When we're rematerializing into a not-quite-right register we already add + // the real definition as an implicit-def, but we should also be marking the + // "official" register as dead, since nothing else is going to use it as a + // result of this remat. Not doing this can affect pressure tracking. NewMI.getOperand(0).setIsDead(true); - if (!NewMIDefinesFullReg) { + bool HasDefMatchingCopy = false; + if (!NewMIImpDefDestReg.empty()) { + // Assert to check MachineOperand*s have not been invalidated. + assert( + NewMIOpCount == NewMI.getNumOperands() && + "Expected NewMI operands not to be appended/removed at this point"); + // If NewMI has an implicit-def of a super-register of the CopyDstReg, + // we must also mark that as dead since it is not going to used as a + // result of this remat. + for (MachineOperand *MO : NewMIImpDefDestReg) { + if (MO->getReg() != CopyDstReg) + MO->setIsDead(true); + else + HasDefMatchingCopy = true; + } + } + + // If NewMI does not already have an implicit-def CopyDstReg add one now. + if (!HasDefMatchingCopy) { NewMI.addOperand(MachineOperand::CreateReg( CopyDstReg, true /*IsDef*/, true /*IsImp*/, false /*IsKill*/)); } diff --git a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir index 13b90e715a7ad..44a2aecdc3672 100644 --- a/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir +++ b/llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir @@ -167,8 +167,6 @@ body: | RET 0, $rax ... --- -; FIXME: `$al = COPY killed %4` should rematerialize as `dead $eax = MOV32r0 ... implicit-def $al` -; not `dead $eax = MOV32r0 ... implicit-def $rax` (as the full $rax is not used). name: rematerialize_superregister_into_subregister_def_with_impdef_physreg body: | bb.0.entry: @@ -176,7 +174,7 @@ body: | ; CHECK: dead $esi = MOV32r0 implicit-def dead $eflags, implicit-def $rsi ; CHECK-NEXT: dead $edx = MOV32r0 implicit-def dead $eflags, implicit-def $rdx ; CHECK-NEXT: FAKE_USE implicit killed $rsi, implicit killed $rdx - ; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def $rax + ; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def dead $rax, implicit-def $al ; CHECK-NEXT: FAKE_USE implicit killed $al ; CHECK-NEXT: $eax = MOV32r0 implicit-def dead $eflags ; CHECK-NEXT: RET 0, $eax From af055e9ce0bff71b4213ac5a75a930af3ae2bde3 Mon Sep 17 00:00:00 2001 From: Benjamin Maxwell Date: Wed, 24 Sep 2025 12:01:21 +0000 Subject: [PATCH 3/4] Rework --- llvm/lib/CodeGen/RegisterCoalescer.cpp | 40 +++++++++----------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp index d117edb6838bd..105d25f5577f3 100644 --- a/llvm/lib/CodeGen/RegisterCoalescer.cpp +++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp @@ -1474,11 +1474,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, // // The implicit-def of the super register may have been reduced to // subregisters depending on the uses. - - TinyPtrVector NewMIImpDefDestReg; - [[maybe_unused]] unsigned NewMIOpCount = NewMI.getNumOperands(); - - SmallVector NewMIImplDefs; + SmallVector, 4> NewMIImplDefs; for (unsigned i = NewMI.getDesc().getNumOperands(), e = NewMI.getNumOperands(); i != e; ++i) { @@ -1486,9 +1482,6 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, if (MO.isReg() && MO.isDef()) { assert(MO.isImplicit()); if (MO.getReg().isPhysical()) { - if (MO.getReg() == DstReg) - NewMIImpDefDestReg.push_back(&MO); - assert(MO.isImplicit() && MO.getReg().isPhysical() && (MO.isDead() || (DefSubIdx && @@ -1496,7 +1489,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, MCRegister((unsigned)NewMI.getOperand(0).getReg())) || TRI->isSubRegisterEq(NewMI.getOperand(0).getReg(), MO.getReg()))))); - NewMIImplDefs.push_back(MO.getReg().asMCReg()); + NewMIImplDefs.push_back({i, MO.getReg().asMCReg()}); } else { assert(MO.getReg() == NewMI.getOperand(0).getReg()); @@ -1649,27 +1642,22 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, NewMI.getOperand(0).setIsDead(true); bool HasDefMatchingCopy = false; - if (!NewMIImpDefDestReg.empty()) { - // Assert to check MachineOperand*s have not been invalidated. - assert( - NewMIOpCount == NewMI.getNumOperands() && - "Expected NewMI operands not to be appended/removed at this point"); - // If NewMI has an implicit-def of a super-register of the CopyDstReg, - // we must also mark that as dead since it is not going to used as a - // result of this remat. - for (MachineOperand *MO : NewMIImpDefDestReg) { - if (MO->getReg() != CopyDstReg) - MO->setIsDead(true); - else - HasDefMatchingCopy = true; - } + for (auto [OpIndex, Reg] : NewMIImplDefs) { + if (Reg != DstReg.asMCReg()) + continue; + // Also, if CopyDstReg is a sub-register of DstReg (and it is defined), we + // must mark DstReg as dead since it is not going to used as a result of + // this remat. + if (DstReg != CopyDstReg) + NewMI.getOperand(OpIndex).setIsDead(true); + else + HasDefMatchingCopy = true; } // If NewMI does not already have an implicit-def CopyDstReg add one now. - if (!HasDefMatchingCopy) { + if (!HasDefMatchingCopy) NewMI.addOperand(MachineOperand::CreateReg( CopyDstReg, true /*IsDef*/, true /*IsImp*/, false /*IsKill*/)); - } // Record small dead def live-ranges for all the subregisters // of the destination register. @@ -1700,7 +1688,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, NewMI.addOperand(MO); SlotIndex NewMIIdx = LIS->getInstructionIndex(NewMI); - for (MCRegister Reg : NewMIImplDefs) { + for (MCRegister Reg : make_second_range(NewMIImplDefs)) { for (MCRegUnit Unit : TRI->regunits(Reg)) if (LiveRange *LR = LIS->getCachedRegUnit(Unit)) LR->createDeadDef(NewMIIdx.getRegSlot(), LIS->getVNInfoAllocator()); From 72282a82c81cf99934dd20cdd7f124ab9f2d6660 Mon Sep 17 00:00:00 2001 From: Benjamin Maxwell Date: Thu, 25 Sep 2025 15:19:42 +0000 Subject: [PATCH 4/4] Fixups --- llvm/lib/CodeGen/RegisterCoalescer.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp index 105d25f5577f3..f7165c535b1d2 100644 --- a/llvm/lib/CodeGen/RegisterCoalescer.cpp +++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp @@ -1474,7 +1474,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, // // The implicit-def of the super register may have been reduced to // subregisters depending on the uses. - SmallVector, 4> NewMIImplDefs; + SmallVector, 4> NewMIImplDefs; for (unsigned i = NewMI.getDesc().getNumOperands(), e = NewMI.getNumOperands(); i != e; ++i) { @@ -1489,7 +1489,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, MCRegister((unsigned)NewMI.getOperand(0).getReg())) || TRI->isSubRegisterEq(NewMI.getOperand(0).getReg(), MO.getReg()))))); - NewMIImplDefs.push_back({i, MO.getReg().asMCReg()}); + NewMIImplDefs.push_back({i, MO.getReg()}); } else { assert(MO.getReg() == NewMI.getOperand(0).getReg()); @@ -1643,7 +1643,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, bool HasDefMatchingCopy = false; for (auto [OpIndex, Reg] : NewMIImplDefs) { - if (Reg != DstReg.asMCReg()) + if (Reg != DstReg) continue; // Also, if CopyDstReg is a sub-register of DstReg (and it is defined), we // must mark DstReg as dead since it is not going to used as a result of @@ -1688,8 +1688,8 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP, NewMI.addOperand(MO); SlotIndex NewMIIdx = LIS->getInstructionIndex(NewMI); - for (MCRegister Reg : make_second_range(NewMIImplDefs)) { - for (MCRegUnit Unit : TRI->regunits(Reg)) + for (Register Reg : make_second_range(NewMIImplDefs)) { + for (MCRegUnit Unit : TRI->regunits(Reg.asMCReg())) if (LiveRange *LR = LIS->getCachedRegUnit(Unit)) LR->createDeadDef(NewMIIdx.getRegSlot(), LIS->getVNInfoAllocator()); }