[PATCH] Modify the register scavenger to operate on reg units when checking liveness
Pedro Artigas
partigas at apple.com
Mon Aug 4 14:54:35 PDT 2014
Hi Jakob,
I updated the change to cover all the comments you made below. Thanks for the review, it helped the code significantly. Patch (first) in text and diff format and comments (way) below.
Thanks,
Pedro
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: opensource6.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140804/05570770/attachment.txt>
-------------- next part --------------
Index: include/llvm/CodeGen/MachineRegisterInfo.h
===================================================================
--- include/llvm/CodeGen/MachineRegisterInfo.h (revision 214774)
+++ include/llvm/CodeGen/MachineRegisterInfo.h (working copy)
@@ -515,8 +515,12 @@
///
/// That function will return NULL if the virtual registers have incompatible
/// constraints.
+ ///
+ /// Note that if ToReg is a physical register the function will replace and
+ /// apply sub registers to ToReg in order to obtain a final/proper physical
+ /// register.
void replaceRegWith(unsigned FromReg, unsigned ToReg);
-
+
/// getVRegDef - Return the machine instr that defines the specified virtual
/// register or null if none is found. This assumes that the code is in SSA
/// form, so there should only be one definition.
Index: include/llvm/CodeGen/RegisterScavenging.h
===================================================================
--- include/llvm/CodeGen/RegisterScavenging.h (revision 214774)
+++ include/llvm/CodeGen/RegisterScavenging.h (working copy)
@@ -34,7 +34,7 @@
MachineRegisterInfo* MRI;
MachineBasicBlock *MBB;
MachineBasicBlock::iterator MBBI;
- unsigned NumPhysRegs;
+ unsigned NumRegUnits;
/// Tracking - True if RegScavenger is currently tracking the liveness of
/// registers.
@@ -58,22 +58,19 @@
/// A vector of information on scavenged registers.
SmallVector<ScavengedInfo, 2> Scavenged;
- /// CalleeSavedrRegs - A bitvector of callee saved registers for the target.
- ///
- BitVector CalleeSavedRegs;
+ /// RegUnitsAvailable - The current state of each reg unit immediatelly
+ /// before MBBI. One bit per register unit. If bit is not set it means any
+ /// register containing that register unit is currently being used.
+ BitVector RegUnitsAvailable;
- /// RegsAvailable - The current state of all the physical registers immediately
- /// before MBBI. One bit per physical register. If bit is set that means it's
- /// available, unset means the register is currently being used.
- BitVector RegsAvailable;
-
// These BitVectors are only used internally to forward(). They are members
// to avoid frequent reallocations.
- BitVector KillRegs, DefRegs;
+ BitVector KillRegUnits, DefRegUnits;
+ BitVector TmpRegUnits;
public:
RegScavenger()
- : MBB(nullptr), NumPhysRegs(0), Tracking(false) {}
+ : MBB(nullptr), NumRegUnits(0), Tracking(false) {}
/// enterBasicBlock - Start tracking liveness from the begin of the specific
/// basic block.
@@ -112,10 +109,10 @@
MachineBasicBlock::iterator getCurrentPosition() const {
return MBBI;
}
+
+ /// isRegUsed - return if a specific register is currently used.
+ bool isRegUsed(unsigned Reg, bool includeReserved = true) const;
- /// getRegsUsed - return all registers currently in use in used.
- void getRegsUsed(BitVector &used, bool includeReserved);
-
/// getRegsAvailable - Return all available registers in the register class
/// in Mask.
BitVector getRegsAvailable(const TargetRegisterClass *RC);
@@ -157,40 +154,29 @@
return scavengeRegister(RegClass, MBBI, SPAdj);
}
- /// setUsed - Tell the scavenger a register is used.
+ /// setRegUsed - Tell the scavenger a register is used.
///
- void setUsed(unsigned Reg);
+ void setRegUsed(unsigned Reg);
private:
/// isReserved - Returns true if a register is reserved. It is never "unused".
bool isReserved(unsigned Reg) const { return MRI->isReserved(Reg); }
- /// isUsed - Test if a register is currently being used. When called by the
- /// isAliasUsed function, we only check isReserved if this is the original
- /// register, not an alias register.
+ /// setUsed / setUnused - Mark the state of one or a number of register units.
///
- bool isUsed(unsigned Reg, bool CheckReserved = true) const {
- return !RegsAvailable.test(Reg) || (CheckReserved && isReserved(Reg));
+ void setUsed(BitVector &RegUnits) {
+ RegUnitsAvailable.reset(RegUnits);
}
-
- /// isAliasUsed - Is Reg or an alias currently in use?
- bool isAliasUsed(unsigned Reg) const;
-
- /// setUsed / setUnused - Mark the state of one or a number of registers.
- ///
- void setUsed(BitVector &Regs) {
- RegsAvailable.reset(Regs);
+ void setUnused(BitVector &RegUnits) {
+ RegUnitsAvailable |= RegUnits;
}
- void setUnused(BitVector &Regs) {
- RegsAvailable |= Regs;
- }
- /// Processes the current instruction and fill the KillRegs and DefRegs bit
- /// vectors.
+ /// Processes the current instruction and fill the KillRegUnits and
+ /// DefRegUnits bit vectors.
void determineKillsAndDefs();
-
- /// Add Reg and all its sub-registers to BV.
- void addRegWithSubRegs(BitVector &BV, unsigned Reg);
-
+
+ /// Add all Reg Units that Reg contains to BV.
+ void addRegUnits(BitVector &BV, unsigned Reg);
+
/// findSurvivorReg - Return the candidate register that is unused for the
/// longest after StartMI. UseMI is set to the instruction where the search
/// stopped.
Index: lib/CodeGen/BranchFolding.cpp
===================================================================
--- lib/CodeGen/BranchFolding.cpp (revision 214774)
+++ lib/CodeGen/BranchFolding.cpp (working copy)
@@ -388,10 +388,8 @@
RS->enterBasicBlock(CurMBB);
if (!CurMBB->empty())
RS->forward(std::prev(CurMBB->end()));
- BitVector RegsLiveAtExit(TRI->getNumRegs());
- RS->getRegsUsed(RegsLiveAtExit, false);
- for (unsigned int i = 0, e = TRI->getNumRegs(); i != e; i++)
- if (RegsLiveAtExit[i])
+ for (unsigned int i = 1, e = TRI->getNumRegs(); i != e; i++)
+ if (RS->isRegUsed(i, false))
NewMBB->addLiveIn(i);
}
}
Index: lib/CodeGen/MachineRegisterInfo.cpp
===================================================================
--- lib/CodeGen/MachineRegisterInfo.cpp (revision 214774)
+++ lib/CodeGen/MachineRegisterInfo.cpp (working copy)
@@ -283,18 +283,25 @@
/// replaceRegWith - Replace all instances of FromReg with ToReg in the
/// machine function. This is like llvm-level X->replaceAllUsesWith(Y),
/// except that it also changes any definitions of the register as well.
+/// If ToReg is a physical register we apply the sub register to obtain the
+/// final/proper physical register.
void MachineRegisterInfo::replaceRegWith(unsigned FromReg, unsigned ToReg) {
assert(FromReg != ToReg && "Cannot replace a reg with itself");
+ const TargetRegisterInfo *TRI = getTargetRegisterInfo();
+
// TODO: This could be more efficient by bulk changing the operands.
for (reg_iterator I = reg_begin(FromReg), E = reg_end(); I != E; ) {
MachineOperand &O = *I;
++I;
- O.setReg(ToReg);
+ if (TargetRegisterInfo::isPhysicalRegister(ToReg)) {
+ O.substPhysReg(ToReg, *TRI);
+ } else {
+ O.setReg(ToReg);
+ }
}
}
-
/// getVRegDef - Return the machine instr that defines the specified virtual
/// register or null if none is found. This assumes that the code is in SSA
/// form, so there should only be one definition.
Index: lib/CodeGen/PrologEpilogInserter.cpp
===================================================================
--- lib/CodeGen/PrologEpilogInserter.cpp (revision 214774)
+++ lib/CodeGen/PrologEpilogInserter.cpp (working copy)
@@ -837,7 +837,8 @@
/// FIXME: Iterating over the instruction stream is unnecessary. We can simply
/// iterate over the vreg use list, which at this point only contains machine
/// operands for which eliminateFrameIndex need a new scratch reg.
-void PEI::scavengeFrameVirtualRegs(MachineFunction &Fn) {
+void
+PEI::scavengeFrameVirtualRegs(MachineFunction &Fn) {
// Run through the instructions and find any virtual registers.
for (MachineFunction::iterator BB = Fn.begin(),
E = Fn.end(); BB != E; ++BB) {
@@ -888,12 +889,16 @@
// Replace this reference to the virtual register with the
// scratch register.
assert (ScratchReg && "Missing scratch register!");
+ MachineRegisterInfo &MRI = Fn.getRegInfo();
Fn.getRegInfo().replaceRegWith(Reg, ScratchReg);
+
+ // Make sure MRI now accounts this register as used.
+ MRI.setPhysRegUsed(ScratchReg);
// Because this instruction was processed by the RS before this
// register was allocated, make sure that the RS now records the
// register as being used.
- RS->setUsed(ScratchReg);
+ RS->setRegUsed(ScratchReg);
}
}
Index: lib/CodeGen/RegisterScavenging.cpp
===================================================================
--- lib/CodeGen/RegisterScavenging.cpp (revision 214774)
+++ lib/CodeGen/RegisterScavenging.cpp (working copy)
@@ -30,20 +30,12 @@
#define DEBUG_TYPE "reg-scavenging"
-/// setUsed - Set the register and its sub-registers as being used.
-void RegScavenger::setUsed(unsigned Reg) {
- for (MCSubRegIterator SubRegs(Reg, TRI, /*IncludeSelf=*/true);
- SubRegs.isValid(); ++SubRegs)
- RegsAvailable.reset(*SubRegs);
+/// setUsed - Set the register units of this register as used.
+void RegScavenger::setRegUsed(unsigned Reg) {
+ for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+ RegUnitsAvailable.reset(*RUI);
}
-bool RegScavenger::isAliasUsed(unsigned Reg) const {
- for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
- if (isUsed(*AI, *AI == Reg))
- return true;
- return false;
-}
-
void RegScavenger::initRegState() {
for (SmallVectorImpl<ScavengedInfo>::iterator I = Scavenged.begin(),
IE = Scavenged.end(); I != IE; ++I) {
@@ -51,8 +43,8 @@
I->Restore = nullptr;
}
- // All registers started out unused.
- RegsAvailable.set();
+ // All register units start out unused.
+ RegUnitsAvailable.set();
if (!MBB)
return;
@@ -60,12 +52,12 @@
// Live-in registers are in use.
for (MachineBasicBlock::livein_iterator I = MBB->livein_begin(),
E = MBB->livein_end(); I != E; ++I)
- setUsed(*I);
+ setRegUsed(*I);
// Pristine CSRs are also unavailable.
BitVector PR = MBB->getParent()->getFrameInfo()->getPristineRegs(MBB);
for (int I = PR.find_first(); I>0; I = PR.find_next(I))
- setUsed(I);
+ setRegUsed(I);
}
void RegScavenger::enterBasicBlock(MachineBasicBlock *mbb) {
@@ -75,7 +67,7 @@
TRI = TM.getRegisterInfo();
MRI = &MF.getRegInfo();
- assert((NumPhysRegs == 0 || NumPhysRegs == TRI->getNumRegs()) &&
+ assert((NumRegUnits == 0 || NumRegUnits == TRI->getNumRegUnits()) &&
"Target changed?");
// It is not possible to use the register scavenger after late optimization
@@ -85,17 +77,11 @@
// Self-initialize.
if (!MBB) {
- NumPhysRegs = TRI->getNumRegs();
- RegsAvailable.resize(NumPhysRegs);
- KillRegs.resize(NumPhysRegs);
- DefRegs.resize(NumPhysRegs);
-
- // Create callee-saved registers bitvector.
- CalleeSavedRegs.resize(NumPhysRegs);
- const MCPhysReg *CSRegs = TRI->getCalleeSavedRegs(&MF);
- if (CSRegs != nullptr)
- for (unsigned i = 0; CSRegs[i]; ++i)
- CalleeSavedRegs.set(CSRegs[i]);
+ NumRegUnits = TRI->getNumRegUnits();
+ RegUnitsAvailable.resize(NumRegUnits);
+ KillRegUnits.resize(NumRegUnits);
+ DefRegUnits.resize(NumRegUnits);
+ TmpRegUnits.resize(NumRegUnits);
}
MBB = mbb;
@@ -104,10 +90,9 @@
Tracking = false;
}
-void RegScavenger::addRegWithSubRegs(BitVector &BV, unsigned Reg) {
- for (MCSubRegIterator SubRegs(Reg, TRI, /*IncludeSelf=*/true);
- SubRegs.isValid(); ++SubRegs)
- BV.set(*SubRegs);
+void RegScavenger::addRegUnits(BitVector &BV, unsigned Reg) {
+ for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+ BV.set(*RUI);
}
void RegScavenger::determineKillsAndDefs() {
@@ -122,12 +107,25 @@
// predicated, conservatively assume "kill" markers do not actually kill the
// register. Similarly ignores "dead" markers.
bool isPred = TII->isPredicated(MI);
- KillRegs.reset();
- DefRegs.reset();
+ KillRegUnits.reset();
+ DefRegUnits.reset();
for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
const MachineOperand &MO = MI->getOperand(i);
- if (MO.isRegMask())
- (isPred ? DefRegs : KillRegs).setBitsNotInMask(MO.getRegMask());
+ if (MO.isRegMask()) {
+
+ TmpRegUnits.clear();
+ for (unsigned RU = 0, RUEnd = TRI->getNumRegUnits(); RU != RUEnd; ++RU) {
+ for (MCRegUnitRootIterator RURI(RU, TRI); RURI.isValid(); ++RURI) {
+ if (MO.clobbersPhysReg(*RURI)) {
+ TmpRegUnits.set(RU);
+ break;
+ }
+ }
+ }
+
+ // Apply the mask.
+ (isPred ? DefRegUnits : KillRegUnits) |= TmpRegUnits;
+ }
if (!MO.isReg())
continue;
unsigned Reg = MO.getReg();
@@ -139,13 +137,13 @@
if (MO.isUndef())
continue;
if (!isPred && MO.isKill())
- addRegWithSubRegs(KillRegs, Reg);
+ addRegUnits(KillRegUnits, Reg);
} else {
assert(MO.isDef());
if (!isPred && MO.isDead())
- addRegWithSubRegs(KillRegs, Reg);
+ addRegUnits(KillRegUnits, Reg);
else
- addRegWithSubRegs(DefRegs, Reg);
+ addRegUnits(DefRegUnits, Reg);
}
}
}
@@ -158,8 +156,8 @@
determineKillsAndDefs();
// Commit the changes.
- setUsed(KillRegs);
- setUnused(DefRegs);
+ setUsed(KillRegUnits);
+ setUnused(DefRegUnits);
}
if (MBBI == MBB->begin()) {
@@ -208,7 +206,7 @@
if (MO.isUse()) {
if (MO.isUndef())
continue;
- if (!isUsed(Reg)) {
+ if (!isRegUsed(Reg)) {
// Check if it's partial live: e.g.
// D0 = insert_subreg D0<undef>, S0
// ... D0
@@ -219,15 +217,23 @@
// insert_subreg around causes both correctness and performance issues.
bool SubUsed = false;
for (MCSubRegIterator SubRegs(Reg, TRI); SubRegs.isValid(); ++SubRegs)
- if (isUsed(*SubRegs)) {
+ if (isRegUsed(*SubRegs)) {
SubUsed = true;
break;
}
- if (!SubUsed) {
+ bool SuperUsed = false;
+ for (MCSuperRegIterator SR(Reg, TRI); SR.isValid(); ++SR) {
+ if (isRegUsed(*SR)) {
+ SuperUsed = true;
+ break;
+ }
+ }
+ if (!SubUsed && !SuperUsed) {
MBB->getParent()->verify(nullptr, "In Register Scavenger");
llvm_unreachable("Using an undefined register!");
}
(void)SubUsed;
+ (void)SuperUsed;
}
} else {
assert(MO.isDef());
@@ -243,23 +249,23 @@
#endif // NDEBUG
// Commit the changes.
- setUnused(KillRegs);
- setUsed(DefRegs);
+ setUnused(KillRegUnits);
+ setUsed(DefRegUnits);
}
-void RegScavenger::getRegsUsed(BitVector &used, bool includeReserved) {
- used = RegsAvailable;
- used.flip();
- if (includeReserved)
- used |= MRI->getReservedRegs();
- else
- used.reset(MRI->getReservedRegs());
+bool RegScavenger::isRegUsed(unsigned Reg, bool includeReserved) const {
+ if (includeReserved && isReserved(Reg))
+ return true;
+ for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI)
+ if (!RegUnitsAvailable.test(*RUI))
+ return true;
+ return false;
}
unsigned RegScavenger::FindUnusedReg(const TargetRegisterClass *RC) const {
for (TargetRegisterClass::iterator I = RC->begin(), E = RC->end();
I != E; ++I)
- if (!isAliasUsed(*I)) {
+ if (!isRegUsed(*I)) {
DEBUG(dbgs() << "Scavenger found unused reg: " << TRI->getName(*I) <<
"\n");
return *I;
@@ -273,13 +279,13 @@
BitVector Mask(TRI->getNumRegs());
for (TargetRegisterClass::iterator I = RC->begin(), E = RC->end();
I != E; ++I)
- if (!isAliasUsed(*I))
+ if (!isRegUsed(*I))
Mask.set(*I);
return Mask;
}
/// findSurvivorReg - Return the candidate register that is unused for the
-/// longest after StargMII. UseMI is set to the instruction where the search
+/// longest after StartMII. UseMI is set to the instruction where the search
/// stopped.
///
/// No more than InstrLimit instructions are inspected.
@@ -375,9 +381,7 @@
}
// Try to find a register that's unused if there is one, as then we won't
- // have to spill. Search explicitly rather than masking out based on
- // RegsAvailable, as RegsAvailable does not take aliases into account.
- // That's what getRegsAvailable() is for.
+ // have to spill.
BitVector Available = getRegsAvailable(RC);
Available &= Candidates;
if (Available.any())
@@ -388,7 +392,7 @@
unsigned SReg = findSurvivorReg(I, Candidates, 25, UseMI);
// If we found an unused register there is no reason to spill it.
- if (!isAliasUsed(SReg)) {
+ if (!isRegUsed(SReg)) {
DEBUG(dbgs() << "Scavenged register: " << TRI->getName(SReg) << "\n");
return SReg;
}
Index: lib/CodeGen/TailDuplication.cpp
===================================================================
--- lib/CodeGen/TailDuplication.cpp (revision 214774)
+++ lib/CodeGen/TailDuplication.cpp (working copy)
@@ -798,11 +798,9 @@
RS->enterBasicBlock(PredBB);
if (!PredBB->empty())
RS->forward(std::prev(PredBB->end()));
- BitVector RegsLiveAtExit(TRI->getNumRegs());
- RS->getRegsUsed(RegsLiveAtExit, false);
for (MachineBasicBlock::livein_iterator I = TailBB->livein_begin(),
E = TailBB->livein_end(); I != E; ++I) {
- if (!RegsLiveAtExit[*I])
+ if (!RS->isRegUsed(*I, false))
// If a register is previously livein to the tail but it's not live
// at the end of predecessor BB, then it should be added to its
// livein list.
Index: lib/Target/XCore/XCoreRegisterInfo.cpp
===================================================================
--- lib/Target/XCore/XCoreRegisterInfo.cpp (revision 214774)
+++ lib/Target/XCore/XCoreRegisterInfo.cpp (working copy)
@@ -98,7 +98,7 @@
MachineBasicBlock &MBB = *MI.getParent();
DebugLoc dl = MI.getDebugLoc();
unsigned ScratchOffset = RS->scavengeRegister(&XCore::GRRegsRegClass, II, 0);
- RS->setUsed(ScratchOffset);
+ RS->setRegUsed(ScratchOffset);
TII.loadImmediate(MBB, II, ScratchOffset, Offset);
switch (MI.getOpcode()) {
@@ -170,12 +170,12 @@
unsigned ScratchBase;
if (OpCode==XCore::STWFI) {
ScratchBase = RS->scavengeRegister(&XCore::GRRegsRegClass, II, 0);
- RS->setUsed(ScratchBase);
+ RS->setRegUsed(ScratchBase);
} else
ScratchBase = Reg;
BuildMI(MBB, II, dl, TII.get(XCore::LDAWSP_ru6), ScratchBase).addImm(0);
unsigned ScratchOffset = RS->scavengeRegister(&XCore::GRRegsRegClass, II, 0);
- RS->setUsed(ScratchOffset);
+ RS->setRegUsed(ScratchOffset);
TII.loadImmediate(MBB, II, ScratchOffset, Offset);
switch (OpCode) {
> On Aug 1, 2014, at 4:36 PM, Jakob Stoklund Olesen <jolesen at apple.com> wrote:
>
>
>> On Jul 31, 2014, at 4:17 PM, Pedro Artigas <partigas at apple.com> wrote:
>>
>> Here is the new version of the patch that addresses the comments by Quentin and Jacob and removes the code for the callee saved bit vector. Compile time measurements indicate no advantage for the changes to tail duplication and the generation of the used bit vector but no regression on average either.
>
> Hi Pedro,
>
> Comments below.
>
>> Index: include/llvm/CodeGen/MachineRegisterInfo.h
>> ===================================================================
>> --- include/llvm/CodeGen/MachineRegisterInfo.h (revision 214448)
>> +++ include/llvm/CodeGen/MachineRegisterInfo.h (working copy)
>> @@ -516,6 +516,23 @@
>> /// That function will return NULL if the virtual registers have incompatible
>> /// constraints.
>> void replaceRegWith(unsigned FromReg, unsigned ToReg);
>> +
>> + /// replaceVRegWithPhysReg - Replace all instances of virtual register
>> + /// fromReg with physical register ToReg, also applies sub-registers in
>> + /// operands to map to final physical register.
>> + ///
>> + /// Example:
>> + ///
>> + /// vreg10 =
>> + /// = vreg10.sub0
>> + /// = vreg10.sub1
>> + ///
>> + /// ==>
>> + ///
>> + /// R5_R6 =
>> + /// = R5
>> + /// = R6
>> + void replaceVRegWithPhysReg(unsigned FromReg, unsigned ToReg);
>
> I don’t think a new function is needed here. Just make the existing replaceRegWith do the right thing when ToReg is a physreg. I can’t imagine a situation where you would want to use the existing functionality with a physreg.
>
> Also, in the implementation please use MachineOperand::substPhysReg() instead of duplicating that functionality.
>
Using the replaceRegsWith() as recommended, also used substPhysReg inside the implementation.
>> @@ -112,6 +111,9 @@
>> MachineBasicBlock::iterator getCurrentPosition() const {
>> return MBBI;
>> }
>> +
>> + /// isRegUsed - return if a specific register is currently used.
>> + bool isRegUsed(int Reg, bool includeReserved);
>>
>
>> + /// isUsed - Test if a register is currently being used. Check reserved
>> + /// registers also if requested.
>> + bool isUsed(unsigned Reg, bool CheckReserved = true) const;
>
> What is the difference between isRegUsed() and isUsed()? Why does one accept a signed Reg argument? Do we need both?
>
No difference, it was a side effect of the old organization, combined both into a single one.
>> + /// setUsed / setUnused - Mark the state of one or a number of register units.
>> ///
>> void setUsed(BitVector &Regs) {
>> - RegsAvailable.reset(Regs);
>> + RegUnitsAvailable.reset(Regs);
>> }
>> void setUnused(BitVector &Regs) {
>> - RegsAvailable |= Regs;
>> + RegUnitsAvailable |= Regs;
>> }
>
> If Regs is a vector of regunits, it shouldn’t be called ‘Regs’.
Changed name to RegUnits.
>
>> Index: lib/CodeGen/MachineRegisterInfo.cpp
>> ===================================================================
>> --- lib/CodeGen/MachineRegisterInfo.cpp (revision 214448)
>> +++ lib/CodeGen/MachineRegisterInfo.cpp (working copy)
>> @@ -294,7 +294,32 @@
>> }
>> }
>>
>> +/// replaceVRegWithPhysReg - Replace all instances of FromReg with ToReg in the
>> +/// machine function. Note that ToReg must be a physical register; that is
>> +/// important as we process the sub-registers applied to the operand to generate
>> +/// a proper phys reg.
>>
>> +void MachineRegisterInfo::replaceVRegWithPhysReg(unsigned FromReg,
>> + unsigned ToReg) {
>> +
>> + const TargetRegisterInfo *TRI = getTargetRegisterInfo();
>> + // TODO: This could be more efficient by bulk changing the operands.
>> + for (reg_iterator I = reg_begin(FromReg), E = reg_end(); I != E; ) {
>> + MachineOperand &O = *I;
>> + ++I;
>> + if (O.getSubReg()) {
>> + unsigned SubReg = TRI->getSubReg(ToReg, O.getSubReg());
>> + assert(SubReg);
>> + setPhysRegUsed(SubReg);
>> + O.setReg(SubReg);
>> + O.setSubReg(0);
>> + } else {
>> + setPhysRegUsed(ToReg);
>> + O.setReg(ToReg);
>> + }
>> + }
>> +}
>
> As noted above, this is duplicating MachineOperand::substPhysReg().
>
> Also, I don’t think this is the right place to call setPhysRegUsed() since other MRI functions don’t do that either. Just call it from the client.
>
Removed function in favor of replaceRegsWith and called setPhysRegUsed from the client as recommended.
>> @@ -888,7 +889,7 @@
>> // Replace this reference to the virtual register with the
>> // scratch register.
>> assert (ScratchReg && "Missing scratch register!");
>> - Fn.getRegInfo().replaceRegWith(Reg, ScratchReg);
>> + Fn.getRegInfo().replaceVRegWithPhysReg(Reg, ScratchReg);
>
> As noted above, this change shouldn’t be necessary.
Change not being made as we are now using plain replaceRegWith().
>
>> +void RegScavenger::addRegUnits(BitVector &BV, BitVector &RBV) {
>> + for (int Reg = RBV.find_first(); Reg != -1; Reg = RBV.find_next(Reg))
>> + if (Reg)
>> + addRegUnits(BV, Reg);
>> +}
>> +
>> +void RegScavenger::subRegUnits(BitVector &BV, BitVector &RBV) {
>> + for (int Reg = RBV.find_first(); Reg != -1; Reg = RBV.find_next(Reg))
>> + if (Reg)
>> + subRegUnits(BV, Reg);
>> +}
>
> These functions look potentially expensive, and the if(Reg) check doesn’t seem right. Why would bit 0 be set? More below.
>
>> @@ -122,12 +139,42 @@
>> // predicated, conservatively assume "kill" markers do not actually kill the
>> // register. Similarly ignores "dead" markers.
>> bool isPred = TII->isPredicated(MI);
>> - KillRegs.reset();
>> - DefRegs.reset();
>> + KillRegUnits.reset();
>> + DefRegUnits.reset();
>> for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
>> const MachineOperand &MO = MI->getOperand(i);
>> - if (MO.isRegMask())
>> - (isPred ? DefRegs : KillRegs).setBitsNotInMask(MO.getRegMask());
>> + if (MO.isRegMask()) {
>> +
>> + // The code below is needed to obtain a proper set of reg units that are
>> + // trully clobbered. As an example consider ARM64 registers X18, X19 and
>> + // the "register" W18_W19. The X18 is not preserved while X19 is;
>> + // therefore W18_W19 is considered clobbered but it has inside it a reg
>> + // unit that is trully preserved (the reg unit for X19). Hence the logic:
>> + //
>> + // ClobberedRegisterUnits - TrullyPreservedRegisterUnits =
>> + // TrullyClobberedRegisterUnits
>> + //
>> + // Is needed in order to obtain a conservative approximation of the
>> + // set that is trully clobbered by the instruction.
>> +
>> + // Get the clobbered registers.
>> + TmpRegs.setBitsInMask(MO.getRegMask());
>> + TmpRegs.flip();
>> +
>> + TmpRegUnits.reset();
>> +
>> + // Add all the clobbered registers as reg units.
>> + addRegUnits(TmpRegUnits, TmpRegs);
>> +
>> + // Get the preserved registers.
>> + TmpRegs.setBitsInMask(MO.getRegMask());
>> +
>> + // Remove all the preserved reg units.
>> + subRegUnits(TmpRegUnits, TmpRegs);
>> +
>> + // Apply the mask.
>> + (isPred ? DefRegUnits : KillRegUnits) |= TmpRegUnits;
>> + }
>
> This looks quite expensive, the addRegUnits / subRegUnits are both O(NumRegUnits * NumRegs).
>
> It should be a lot faster to check if the root registers are preserved for each regunit.
Done as recommended, code is smaller now and likely faster.
>
>
>> void RegScavenger::getRegsUsed(BitVector &used, bool includeReserved) {
>> - used = RegsAvailable;
>> - used.flip();
>> + TmpRegUnits = RegUnitsAvailable;
>> + TmpRegUnits.flip();
>
> I don’t think any callers actually need this whole vector, and it is quite expensive to compute. You should remove this function and change the callers to use isRegUsed() instead.
>
Done as recommended.
> Thanks,
> /jakob
>
More information about the llvm-commits
mailing list