Skip to content

Commit d357e96

Browse files
authored
[RegisterCoalescer] Mark implicit-defs of super-registers as dead in remat (#159110)
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. Note: This issue is latent right now, but is exposed when #134408 is applied, as it results in the register pressure being incorrectly calculated (unless this patch is applied too). I think this change is in line with past fixes in this area, notably: 059cead 69cd121
1 parent 6567070 commit d357e96

File tree

2 files changed

+45
-13
lines changed

2 files changed

+45
-13
lines changed

llvm/lib/CodeGen/RegisterCoalescer.cpp

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1475,28 +1475,22 @@ bool RegisterCoalescer::reMaterializeDef(const CoalescerPair &CP,
14751475
//
14761476
// The implicit-def of the super register may have been reduced to
14771477
// subregisters depending on the uses.
1478-
1479-
bool NewMIDefinesFullReg = false;
1480-
1481-
SmallVector<MCRegister, 4> NewMIImplDefs;
1478+
SmallVector<std::pair<unsigned, Register>, 4> NewMIImplDefs;
14821479
for (unsigned i = NewMI.getDesc().getNumOperands(),
14831480
e = NewMI.getNumOperands();
14841481
i != e; ++i) {
14851482
MachineOperand &MO = NewMI.getOperand(i);
14861483
if (MO.isReg() && MO.isDef()) {
14871484
assert(MO.isImplicit());
14881485
if (MO.getReg().isPhysical()) {
1489-
if (MO.getReg() == DstReg)
1490-
NewMIDefinesFullReg = true;
1491-
14921486
assert(MO.isImplicit() && MO.getReg().isPhysical() &&
14931487
(MO.isDead() ||
14941488
(DefSubIdx &&
14951489
((TRI->getSubReg(MO.getReg(), DefSubIdx) ==
14961490
MCRegister((unsigned)NewMI.getOperand(0).getReg())) ||
14971491
TRI->isSubRegisterEq(NewMI.getOperand(0).getReg(),
14981492
MO.getReg())))));
1499-
NewMIImplDefs.push_back(MO.getReg().asMCReg());
1493+
NewMIImplDefs.push_back({i, MO.getReg()});
15001494
} else {
15011495
assert(MO.getReg() == NewMI.getOperand(0).getReg());
15021496

@@ -1641,12 +1635,30 @@ bool RegisterCoalescer::reMaterializeDef(const CoalescerPair &CP,
16411635
// been asked for. If so it must implicitly define the whole thing.
16421636
assert(DstReg.isPhysical() &&
16431637
"Only expect virtual or physical registers in remat");
1638+
1639+
// When we're rematerializing into a not-quite-right register we already add
1640+
// the real definition as an implicit-def, but we should also be marking the
1641+
// "official" register as dead, since nothing else is going to use it as a
1642+
// result of this remat. Not doing this can affect pressure tracking.
16441643
NewMI.getOperand(0).setIsDead(true);
16451644

1646-
if (!NewMIDefinesFullReg) {
1645+
bool HasDefMatchingCopy = false;
1646+
for (auto [OpIndex, Reg] : NewMIImplDefs) {
1647+
if (Reg != DstReg)
1648+
continue;
1649+
// Also, if CopyDstReg is a sub-register of DstReg (and it is defined), we
1650+
// must mark DstReg as dead since it is not going to used as a result of
1651+
// this remat.
1652+
if (DstReg != CopyDstReg)
1653+
NewMI.getOperand(OpIndex).setIsDead(true);
1654+
else
1655+
HasDefMatchingCopy = true;
1656+
}
1657+
1658+
// If NewMI does not already have an implicit-def CopyDstReg add one now.
1659+
if (!HasDefMatchingCopy)
16471660
NewMI.addOperand(MachineOperand::CreateReg(
16481661
CopyDstReg, true /*IsDef*/, true /*IsImp*/, false /*IsKill*/));
1649-
}
16501662

16511663
// Record small dead def live-ranges for all the subregisters
16521664
// of the destination register.
@@ -1677,8 +1689,8 @@ bool RegisterCoalescer::reMaterializeDef(const CoalescerPair &CP,
16771689
NewMI.addOperand(MO);
16781690

16791691
SlotIndex NewMIIdx = LIS->getInstructionIndex(NewMI);
1680-
for (MCRegister Reg : NewMIImplDefs) {
1681-
for (MCRegUnit Unit : TRI->regunits(Reg))
1692+
for (Register Reg : make_second_range(NewMIImplDefs)) {
1693+
for (MCRegUnit Unit : TRI->regunits(Reg.asMCReg()))
16821694
if (LiveRange *LR = LIS->getCachedRegUnit(Unit))
16831695
LR->createDeadDef(NewMIIdx.getRegSlot(), LIS->getVNInfoAllocator());
16841696
}

llvm/test/CodeGen/X86/rematerialize-sub-super-reg.mir

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,5 +165,25 @@ body: |
165165
bb.3:
166166
$rax = COPY %t3
167167
RET 0, $rax
168-
169168
...
169+
---
170+
name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
171+
body: |
172+
bb.0.entry:
173+
; CHECK-LABEL: name: rematerialize_superregister_into_subregister_def_with_impdef_physreg
174+
; CHECK: dead $esi = MOV32r0 implicit-def dead $eflags, implicit-def $rsi
175+
; CHECK-NEXT: dead $edx = MOV32r0 implicit-def dead $eflags, implicit-def $rdx
176+
; CHECK-NEXT: FAKE_USE implicit killed $rsi, implicit killed $rdx
177+
; CHECK-NEXT: dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def dead $rax, implicit-def $al
178+
; CHECK-NEXT: FAKE_USE implicit killed $al
179+
; CHECK-NEXT: $eax = MOV32r0 implicit-def dead $eflags
180+
; CHECK-NEXT: RET 0, $eax
181+
undef %1.sub_32bit:gr64_with_sub_8bit = MOV32r0 implicit-def dead $eflags, implicit-def %1
182+
$rsi = COPY %1
183+
$rdx = COPY %1
184+
FAKE_USE implicit killed $rsi, implicit killed $rdx
185+
%4:gr8 = COPY killed %1.sub_8bit
186+
$al = COPY killed %4
187+
FAKE_USE implicit killed $al
188+
$eax = MOV32r0 implicit-def dead $eflags
189+
RET 0, killed $eax

0 commit comments

Comments
 (0)