Skip to content

Commit

Permalink
Revert "Reland "RegisterCoalescer: Add implicit-def of super register…
Browse files Browse the repository at this point in the history 
… when coalescing SUBREG_TO_REG" (llvm#123632)"

There's a regression with one of the bootstrap builds for x86.
I'll revert this while I investigate.

This reverts commit 4df6d3d.
  • Loading branch information
@sdesmalen-arm
sdesmalen-arm committed Jan 22, 2025
1 parent 974f678 commit 6b1db79
Show file tree
Hide file tree
Showing 24 changed files with 154 additions and 1,263 deletions.
81 changes: 16 additions & 65 deletions llvm/lib/CodeGen/RegisterCoalescer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,11 +306,7 @@ namespace {
/// number if it is not zero. If DstReg is a physical register and the
/// existing subregister number of the def / use being updated is not zero,
/// make sure to set it to the correct physical subregister.
///
/// If \p IsSubregToReg, we are coalescing a DstReg = SUBREG_TO_REG
/// SrcReg. This introduces an implicit-def of DstReg on coalesced users.
void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx,
bool IsSubregToReg);
void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx);

/// If the given machine operand reads only undefined lanes add an undef
/// flag.
Expand Down Expand Up @@ -1434,7 +1430,6 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,

// CopyMI may have implicit operands, save them so that we can transfer them
// over to the newly materialized instruction after CopyMI is removed.
LaneBitmask NewMIImplicitOpsMask;
SmallVector<MachineOperand, 4> ImplicitOps;
ImplicitOps.reserve(CopyMI->getNumOperands() -
CopyMI->getDesc().getNumOperands());
Expand All @@ -1448,9 +1443,6 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
(MO.getSubReg() == 0 && MO.getReg() == DstOperand.getReg())) &&
"unexpected implicit virtual register def");
ImplicitOps.push_back(MO);
if (MO.isDef() && MO.getReg().isVirtual() &&
MRI->shouldTrackSubRegLiveness(DstReg))
NewMIImplicitOpsMask |= MRI->getMaxLaneMaskForVReg(MO.getReg());
}
}

Expand Down Expand Up @@ -1493,11 +1485,14 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
} else {
assert(MO.getReg() == NewMI.getOperand(0).getReg());

// If lanemasks need to be tracked, compile the lanemask of the NewMI
// implicit def operands to avoid subranges for the super-regs from
// being removed by code later on in this function.
if (MRI->shouldTrackSubRegLiveness(MO.getReg()))
NewMIImplicitOpsMask |= MRI->getMaxLaneMaskForVReg(MO.getReg());
// We're only expecting another def of the main output, so the range
// should get updated with the regular output range.
//
// FIXME: The range updating below probably needs updating to look at
// the super register if subranges are tracked.
assert(!MRI->shouldTrackSubRegLiveness(DstReg) &&
"subrange update for implicit-def of super register may not be "
"properly handled");
}
}
}
Expand All @@ -1521,7 +1516,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
MRI->setRegClass(DstReg, NewRC);

// Update machine operands and add flags.
updateRegDefsUses(DstReg, DstReg, DstIdx, false);
updateRegDefsUses(DstReg, DstReg, DstIdx);
NewMI.getOperand(0).setSubReg(NewIdx);
// updateRegDefUses can add an "undef" flag to the definition, since
// it will replace DstReg with DstReg.DstIdx. If NewIdx is 0, make
Expand Down Expand Up @@ -1597,8 +1592,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
CurrIdx.getRegSlot(NewMI.getOperand(0).isEarlyClobber());
VNInfo::Allocator &Alloc = LIS->getVNInfoAllocator();
for (LiveInterval::SubRange &SR : DstInt.subranges()) {
if ((SR.LaneMask & DstMask).none() &&
(SR.LaneMask & NewMIImplicitOpsMask).none()) {
if ((SR.LaneMask & DstMask).none()) {
LLVM_DEBUG(dbgs()
<< "Removing undefined SubRange "
<< PrintLaneMask(SR.LaneMask) << " : " << SR << "\n");
Expand Down Expand Up @@ -1863,7 +1857,7 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx,
}

void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
unsigned SubIdx, bool IsSubregToReg) {
unsigned SubIdx) {
bool DstIsPhys = DstReg.isPhysical();
LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg);

Expand All @@ -1883,14 +1877,6 @@ void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
}
}

// If DstInt already has a subrange for the unused lanes, then we shouldn't
// create duplicate subranges when we update the interval for unused lanes.
LaneBitmask DefinedLanes;
if (DstInt && MRI->shouldTrackSubRegLiveness(DstReg)) {
for (LiveInterval::SubRange &SR : DstInt->subranges())
DefinedLanes |= SR.LaneMask;
}

SmallPtrSet<MachineInstr*, 8> Visited;
for (MachineRegisterInfo::reg_instr_iterator
I = MRI->reg_instr_begin(SrcReg), E = MRI->reg_instr_end();
Expand All @@ -1914,19 +1900,15 @@ void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
if (DstInt && !Reads && SubIdx && !UseMI->isDebugInstr())
Reads = DstInt->liveAt(LIS->getInstructionIndex(*UseMI));

bool FullDef = true;

// Replace SrcReg with DstReg in all UseMI operands.
for (unsigned Op : Ops) {
MachineOperand &MO = UseMI->getOperand(Op);

// Adjust <undef> flags in case of sub-register joins. We don't want to
// turn a full def into a read-modify-write sub-register def and vice
// versa.
if (SubIdx && MO.isDef()) {
if (SubIdx && MO.isDef())
MO.setIsUndef(!Reads);
FullDef = false;
}

// A subreg use of a partially undef (super) register may be a complete
// undef use now and then has to be marked that way.
Expand Down Expand Up @@ -1959,32 +1941,6 @@ void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
MO.substVirtReg(DstReg, SubIdx, *TRI);
}

if (IsSubregToReg && !FullDef) {
// If the coalesed instruction doesn't fully define the register, we need
// to preserve the original super register liveness for SUBREG_TO_REG.
//
// We pretended SUBREG_TO_REG was a regular copy for coalescing purposes,
// but it introduces liveness for other subregisters. Downstream users may
// have been relying on those bits, so we need to ensure their liveness is
// captured with a def of other lanes.

if (DstInt && MRI->shouldTrackSubRegLiveness(DstReg)) {
assert(DstInt->hasSubRanges() &&
"SUBREG_TO_REG should have resulted in subrange");
LaneBitmask DstMask = MRI->getMaxLaneMaskForVReg(DstInt->reg());
LaneBitmask UsedLanes = TRI->getSubRegIndexLaneMask(SubIdx);
LaneBitmask UnusedLanes = DstMask & ~UsedLanes & ~DefinedLanes;
if ((UnusedLanes).any()) {
BumpPtrAllocator &Allocator = LIS->getVNInfoAllocator();
DstInt->createSubRangeFrom(Allocator, UnusedLanes, *DstInt);
DefinedLanes |= UnusedLanes;
}
}

MachineInstrBuilder MIB(*MF, UseMI);
MIB.addReg(DstReg, RegState::ImplicitDefine);
}

LLVM_DEBUG({
dbgs() << "\t\tupdated: ";
if (!UseMI->isDebugInstr())
Expand Down Expand Up @@ -2186,8 +2142,6 @@ bool RegisterCoalescer::joinCopy(
});
}

const bool IsSubregToReg = CopyMI->isSubregToReg();

ShrinkMask = LaneBitmask::getNone();
ShrinkMainRange = false;

Expand Down Expand Up @@ -2257,12 +2211,9 @@ bool RegisterCoalescer::joinCopy(

// Rewrite all SrcReg operands to DstReg.
// Also update DstReg operands to include DstIdx if it is set.
if (CP.getDstIdx()) {
assert(!IsSubregToReg && "can this happen?");
updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx(), false);
}
updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx(),
IsSubregToReg);
if (CP.getDstIdx())
updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx());
updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx());

// Shrink subregister ranges if necessary.
if (ShrinkMask.any()) {
Expand Down
Loading

0 comments on commit 6b1db79

Please sign in to comment.