Page MenuHomePhabricator
Differential D20531 Diff 58596 lib/CodeGen/MachineCopyPropagation.cpp

Changeset View

Standalone View

View Options

lib/CodeGen/MachineCopyPropagation.cpp

Show First 20 Lines • Show All 173 Lines • ▼ Show 20 Lines
void MachineCopyPropagation::CopyPropagateBlock(MachineBasicBlock &MBB) { void MachineCopyPropagation::CopyPropagateBlock(MachineBasicBlock &MBB) {
DEBUG(dbgs() << "MCP: CopyPropagateBlock " << MBB.getName() << "\n"); DEBUG(dbgs() << "MCP: CopyPropagateBlock " << MBB.getName() << "\n");
for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E; ) { for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E; ) {
MachineInstr *MI = &*I; MachineInstr *MI = &*I;
++I; ++I;
const MachineOperand *RegMask = nullptr;
if (MI->isCopy()) { if (MI->isCopy()) {
unsigned Def = MI->getOperand(0).getReg(); unsigned Def = MI->getOperand(0).getReg();
unsigned Src = MI->getOperand(1).getReg(); unsigned Src = MI->getOperand(1).getReg();
assert(!TargetRegisterInfo::isVirtualRegister(Def) && assert(!TargetRegisterInfo::isVirtualRegister(Def) &&
!TargetRegisterInfo::isVirtualRegister(Src) && !TargetRegisterInfo::isVirtualRegister(Src) &&
"MachineCopyPropagation should be run after register allocation!"); "MachineCopyPropagation should be run after register allocation!");
Show All 34 Lines if (MI->isCopy()) {
// If 'Def' is previously source of another copy, then this earlier copy's // If 'Def' is previously source of another copy, then this earlier copy's
// source is no longer available. e.g. // source is no longer available. e.g.
// %xmm9<def> = copy %xmm2 // %xmm9<def> = copy %xmm2
// ... // ...
// %xmm2<def> = copy %xmm0 // %xmm2<def> = copy %xmm0
// ... // ...
// %xmm2<def> = copy %xmm9 // %xmm2<def> = copy %xmm9
ClobberRegister(Def); ClobberRegister(Def);
// Handle any second implicit def operand.
if (MI->getNumOperands() == 3 && MI->getOperand(2).isDef())
ClobberRegister(MI->getOperand(2).getReg());
MatzeBUnsubmitted
Not Done
ReplyInline Actions

This looks strange. In theory we can see any number of additional implicit operands on a COPY. I assume we were just to not hit any issues because in todays practice we only add implicit defs and uses of the superregister when a subregister was used (see code in VirtRegRewriter).

Do you actually need this bugfix as part of this patch? As far as I can see COPY instructions are not renamed here so we should not run into any new problems here.

(It would still be good to upstream this fix in a separate commit through and use a loop so we also catch the case with 4 operands when we had a subregister def and use at the COPY).

MatzeB: This looks strange. In theory we can see any number of additional implicit operands on a COPY.
jonpaAuthorUnsubmitted
Not Done
ReplyInline Actions

I thought that it was kind of a bug to not ignore an implicit-def operand regardless of anything else.

As I recall, it was also necessary to handle during my experiments, either because the MachineVerifier complained that a register was used without a def (if that COPY was removed), or because I wanted to avoid bothering with subregisters all together (changing just one subregister is not a good idea). Not sure exactly which of these I ran into.

jonpa: I thought that it was kind of a bug to not ignore an implicit-def operand regardless of…
// Remember Def is defined by the copy. // Remember Def is defined by the copy.
for (MCSubRegIterator SR(Def, TRI, /*IncludeSelf=*/true); SR.isValid(); for (MCSubRegIterator SR(Def, TRI, /*IncludeSelf=*/true); SR.isValid();
++SR) { ++SR) {
CopyMap[*SR] = MI; CopyMap[*SR] = MI;
AvailCopyMap[*SR] = MI; AvailCopyMap[*SR] = MI;
} }
// Remember source that's copied to Def. Once it's clobbered, then // Remember source that's copied to Def. Once it's clobbered, then
// it's no longer available for copy propagation. // it's no longer available for copy propagation.
RegList &DestList = SrcMap[Src]; RegList &DestList = SrcMap[Src];
if (std::find(DestList.begin(), DestList.end(), Def) == DestList.end()) if (std::find(DestList.begin(), DestList.end(), Def) == DestList.end())
DestList.push_back(Def); DestList.push_back(Def);
continue;
} }
else {
// Not a copy. // Not a copy.
SmallVector<unsigned, 2> Defs; SmallVector<unsigned, 2> Defs;
const MachineOperand *RegMask = nullptr; for (unsigned MOIdx = 0; MOIdx < MI->getNumOperands() ; ++MOIdx) {
for (const MachineOperand &MO : MI->operands()) { MachineOperand &MO = MI->getOperand(MOIdx);
if (MO.isRegMask()) if (MO.isRegMask())
RegMask = &MO; RegMask = &MO;
if (!MO.isReg()) if (!MO.isReg())
continue; continue;
unsigned Reg = MO.getReg(); unsigned Reg = MO.getReg();
if (!Reg) if (!Reg)
continue; continue;
assert(!TargetRegisterInfo::isVirtualRegister(Reg) && assert(!TargetRegisterInfo::isVirtualRegister(Reg) &&
"MachineCopyPropagation should be run after register allocation!"); "MachineCopyPropagation should be run after register "
"allocation!");
if (MO.isDef()) { if (MO.isDef()) {
Defs.push_back(Reg); Defs.push_back(Reg);
continue; continue;
} }
// If 'Reg' is killed and is defined by a previous copy of a
// register that is also in the right register class, use that
// earlier source register instead. This may make that previous
junbumlUnsubmitted
Not Done
ReplyInline Actions

insted -> instead

junbuml: insted -> instead
// copy removable. Skipping subregs and multiple uses for now.
if (!MRI->isReserved(Reg) &&
junbumlUnsubmitted
Not Done
ReplyInline Actions

You may intended this continue in outside of this for loop ?

junbuml: You may intended this continue in outside of this for loop ?
MO.readsReg() && MO.isKill() && !MO.isTied() &&
!TII->isGenericOpcode(MI->getOpcode())) {
Reg2MIMap::iterator CI = AvailCopyMap.find(MO.getReg());
if (CI != AvailCopyMap.end()) {
MachineInstr &PrevCopy = *CI->second;
junbumlUnsubmitted
Not Done
ReplyInline Actions

I doubt if you can simply replace MO with PrevCopySrc whenever the register class is matched. What if either MO or PrevCopySrc is reserved ?

junbuml: I doubt if you can simply replace MO with PrevCopySrc whenever the register class is matched.
MachineOperand &PrevCopySrc = PrevCopy.getOperand(1);
const TargetRegisterClass *RC =
TII->getRegClass(MI->getDesc(), MOIdx, TRI, *MBB.getParent());
if (!MRI->isReserved(PrevCopySrc.getReg()) &&
!PrevCopySrc.getSubReg() &&
RC != nullptr && RC->contains(PrevCopySrc.getReg())) {
MO.setReg(PrevCopySrc.getReg());
continue;
}
MatzeBUnsubmitted
Not Done
ReplyInline Actions

I found that there are examples where you can only rename some of the operands (the reg may be used multiple times, but on of the inputs is tied to an output and cannot be renamed). It is semantically questionable to rename only parts of the operands (and you will not get any scheduling benefits or dead copy removal opportunities anyway in this case).

MatzeB: I found that there are examples where you can only rename some of the operands (the reg may be…
}
}
// If 'Reg' is defined by a copy, the copy is no longer a candidate // If 'Reg' is defined by a copy, the copy is no longer a candidate
// for elimination. // for elimination.
for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) { for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
Reg2MIMap::iterator CI = CopyMap.find(*AI); Reg2MIMap::iterator CI = CopyMap.find(*AI);
if (CI != CopyMap.end()) { if (CI != CopyMap.end()) {
DEBUG(dbgs() << "MCP: Copy is used - not dead: "; CI->second->dump()); DEBUG(dbgs() << "MCP: Copy is used - not dead: "; CI->second->dump());
MaybeDeadCopies.remove(CI->second); MaybeDeadCopies.remove(CI->second);
} }
} }
// Treat undef use like defs for copy propagation but not for // Treat undef use like defs for copy propagation but not for
// dead copy. We would need to do a liveness check to be sure the copy // dead copy. We would need to do a liveness check to be sure the copy
// is dead for undef uses. // is dead for undef uses.
// The backends are allowed to do whatever they want with undef value // The backends are allowed to do whatever they want with undef value
// and we cannot be sure this register will not be rewritten to break // and we cannot be sure this register will not be rewritten to break
// some false dependencies for the hardware for instance. // some false dependencies for the hardware for instance.
if (MO.isUndef()) if (MO.isUndef())
Defs.push_back(Reg); Defs.push_back(Reg);
} }
// The instruction has a register mask operand which means that it clobbers // Any previous copy definition or reading the Defs is no longer available.
// a large set of registers. Treat clobbered registers the same way as for (unsigned Reg : Defs)
// defined registers. ClobberRegister(Reg);
if (RegMask) { }
// Erase any MaybeDeadCopies whose destination register is clobbered.
// Erase any MaybeDeadCopies whose destination register was clobbered.
for (SmallSetVector<MachineInstr *, 8>::iterator DI = for (SmallSetVector<MachineInstr *, 8>::iterator DI =
MaybeDeadCopies.begin(); MaybeDeadCopies.begin();
DI != MaybeDeadCopies.end();) { DI != MaybeDeadCopies.end();) {
MachineInstr *MaybeDead = *DI; MachineInstr *MaybeDead = *DI;
unsigned Reg = MaybeDead->getOperand(0).getReg(); if (MaybeDead == MI) {
assert(!MRI->isReserved(Reg));
if (!RegMask->clobbersPhysReg(Reg)) {
++DI; ++DI;
continue; continue;
} }
unsigned Reg = MaybeDead->getOperand(0).getReg();
assert(!MRI->isReserved(Reg));
if (MI->definesRegister(Reg, TRI))
DEBUG(dbgs() << "MCP: Removing copy due to def: ";
MaybeDead->dump());
else if (RegMask && RegMask->clobbersPhysReg(Reg))
junbumlUnsubmitted
Not Done
ReplyInline Actions

Cann't you use MI->definesRegister() here instead of tracking defs through Clobbers.

junbuml: Cann't you use MI->definesRegister() here instead of tracking defs through Clobbers.
DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: "; DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
MaybeDead->dump()); MaybeDead->dump());
else {
++DI;
continue;
}
// erase() will return the next valid iterator pointing to the next // erase() will return the next valid iterator pointing to the next
// element after the erased one. // element after the erased one.
DI = MaybeDeadCopies.erase(DI); DI = MaybeDeadCopies.erase(DI);
MaybeDead->eraseFromParent(); MaybeDead->eraseFromParent();
Changed = true; Changed = true;
++NumDeletes; ++NumDeletes;
} }
if (RegMask) {
removeClobberedRegsFromMap(AvailCopyMap, *RegMask); removeClobberedRegsFromMap(AvailCopyMap, *RegMask);
removeClobberedRegsFromMap(CopyMap, *RegMask); removeClobberedRegsFromMap(CopyMap, *RegMask);
for (SourceMap::iterator I = SrcMap.begin(), E = SrcMap.end(), Next; for (SourceMap::iterator I = SrcMap.begin(), E = SrcMap.end(), Next;
I != E; I = Next) { I != E; I = Next) {
Next = std::next(I); Next = std::next(I);
if (RegMask->clobbersPhysReg(I->first)) { if (RegMask->clobbersPhysReg(I->first)) {
removeRegsFromMap(AvailCopyMap, I->second, *TRI); removeRegsFromMap(AvailCopyMap, I->second, *TRI);
SrcMap.erase(I); SrcMap.erase(I);
} }
} }
} }
// Any previous copy definition or reading the Defs is no longer available.
for (unsigned Reg : Defs)
ClobberRegister(Reg);
} }
// If MBB doesn't have successors, delete the copies whose defs are not used. // If MBB doesn't have successors, delete the copies whose defs are not used.
// If MBB does have successors, then conservative assume the defs are live-out // If MBB does have successors, then conservative assume the defs are live-out
// since we don't want to trust live-in lists. // since we don't want to trust live-in lists.
if (MBB.succ_empty()) { if (MBB.succ_empty()) {
for (MachineInstr *MaybeDead : MaybeDeadCopies) { for (MachineInstr *MaybeDead : MaybeDeadCopies) {
assert(!MRI->isReserved(MaybeDead->getOperand(0).getReg())); assert(!MRI->isReserved(MaybeDead->getOperand(0).getReg()));
Show All 28 Lines