[llvm-commits] [llvm] r47046 - in /llvm/trunk: include/llvm/CodeGen/LiveIntervalAnalysis.h lib/CodeGen/SimpleRegisterCoalescing.cpp lib/CodeGen/SimpleRegisterCoalescing.h
Chris Lattner
clattner at apple.com
Tue Feb 12 21:56:03 PST 2008
On Feb 12, 2008, at 7:01 PM, Evan Cheng wrote:
> Author: evancheng
> Date: Tue Feb 12 21:01:43 2008
> New Revision: 47046
>
> URL: http://llvm.org/viewvc/llvm-project?rev=47046&view=rev
> Log:
> Initial support for copy elimination by commuting its definition MI.
Yay, thanks for tackling this Evan!
>
> This speeds up FreeBench/neural by 29%, Olden/bh by 12%, oopack_v1p8
> by 53%.
Very nice, does it also help shootout/fib?
>
> + /// ReplaceMachineInstrInMaps - Replacing a machine instr with
> a new one in
> + /// maps used by register allocator.
> + void ReplaceMachineInstrInMaps(MachineInstr *MI, MachineInstr
> *NewMI) {
> + Mi2IndexMap::iterator mi2i = mi2iMap_.find(MI);
> + if (mi2i != mi2iMap_.end()) {
Please change this to:
if (mi2i == mi2iMap_.end())
return;
...
Just to avoid indentation.
>
> + i2miMap_[mi2i->second/InstrSlots::NUM] = NewMI;
>
> + Mi2IndexMap::const_iterator it = mi2iMap_.find(MI);
> + assert(it != mi2iMap_.end() && "Invalid instruction!");
> + unsigned Index = it->second;
> + mi2iMap_.erase(MI);
This would avoid another lookup if you used .erase(it);
> +bool
> SimpleRegisterCoalescing::RemoveCopyByCommutingDef(LiveInterval &IntA,
> +
> LiveInterval &IntB,
> +
> MachineInstr *CopyMI) {
...
> + // Get the location that B is defined at. Two options: either
> this value has
> + // an unknown definition point or it is defined at CopyIdx. If
> unknown, we
> + // can't process it.
> + if (!BValNo->reg) return false;
> + assert(BValNo->def == CopyIdx && "Copy doesn't define the value?");
> +
> + // AValNo is the value number in A that defines the copy, A3 in
> the example.
> + LiveInterval::iterator ALR =
> IntA.FindLiveRangeContaining(CopyIdx-1);
> + VNInfo *AValNo = ALR->valno;
This idiom happens a lot in the preexisting code. Please use:
VNInfo *AValNo = IntA.FindLiveRangeContaining(CopyIdx-1)->valno;
Or better yet, add a new method that returns valno directly.
> + int Idx = -1;
"Idx" is too generic of a name for a variable with this long of
lifetime, please name it something more descriptive. What type of idx
is it?
>
> + for (unsigned i = 0, e = DefMI->getNumOperands(); i != e; ++i) {
This loop needs a comment. What are you trying to find with this
loop? Maybe it should be moved out to its own function which just
returns idx? This would force you to name it, which would describe
what it does :)
>
> + MachineOperand &MO = DefMI->getOperand(i);
> + if (!MO.isRegister()) continue;
> + unsigned Reg = MO.getReg();
> + if (Reg && TargetRegisterInfo::isVirtualRegister(Reg)) {
How about:
if (Reg == 0 || !isvreg) continue;
> + if (rep(Reg) == IntA.reg) {
> + // If the dest register comes from an interval other than
> IntA, we
> + // can't handle this.
> + if (Reg != IntA.reg)
> + return false;
I must be missing something here, how do you know this is a def operand?
Another random question unrelated to this code: now that we have
efficient RAUW, can we eliminate the 'rep' mapping stuff and just
RAUW vregs as they are coallesced?
>
> + continue;
> + }
> + if (Idx != -1)
> + // FIXME: Being overly careful here. We just need to figure
> out the
> + // which register operand will become the new def.
> + return false;
> + Idx = i;
> + }
> + }
> + if (Idx == -1)
> + // Something like %reg1024 = add %reg1024, %reg1024
> + return false;
> +
> + MachineOperand &MO = DefMI->getOperand(Idx);
> + unsigned NewReg = MO.getReg();
> + if (rep(NewReg) != IntB.reg || !MO.isKill())
> + return false;
This logic would make more sense if I knew what Idx indicates and why
you're rejecting these cases :). Also, MO has a long lifetime, please
name it "DefMO" or something else more descriptive.
> + // Make sure there are no other definitions of IntB that would
> reach the
> + // uses which the new definition can reach.
> + for (LiveInterval::iterator AI = IntA.begin(), AE = IntA.end();
> + AI != AE; ++AI) {
> + if (AI->valno != AValNo) continue;
>
> + LiveInterval::Ranges::iterator BI =
> + std::upper_bound(IntB.ranges.begin(), IntB.ranges.end(), AI-
> >start);
> + if (BI != IntB.ranges.begin())
> + --BI;
> + for (; BI != IntB.ranges.end() && AI->end >= BI->start; ++BI) {
> + if (BI->valno == BLR->valno)
> + continue;
> + if (BI->start <= AI->start && BI->end > AI->start)
> + return false;
> + if (BI->start > AI->start && BI->start < AI->end)
> + return false;
> + }
Please split this out into a static function or a method on
liveinterval.
>
> + }
> +
> + // Commute def machine instr.
How about: at this point we have decided that it is legal to do this
transformation. Start by commuting the instruction.
>
> + MachineBasicBlock *MBB = DefMI->getParent();
> + MachineInstr *NewMI = tii_->commuteInstruction(DefMI);
> + if (NewMI != DefMI) {
> + li_->ReplaceMachineInstrInMaps(DefMI, NewMI);
> + MBB->insert(DefMI, NewMI);
> + MBB->erase(DefMI);
> + }
> + unsigned OpIdx = NewMI->findRegisterUseOperandIdx(IntA.reg);
> + NewMI->getOperand(OpIdx).setIsKill();
> +
> + // Update uses of IntA of the specific Val# with IntB.
> + bool BHasPHIKill = BValNo->hasPHIKill;
> + SmallVector<VNInfo*, 4> BDeadValNos;
> + SmallVector<unsigned, 4> BKills;
> + std::map<unsigned, unsigned> BExtend;
> + for (MachineRegisterInfo::use_iterator UI = mri_-
> >use_begin(IntA.reg),
> + UE = mri_->use_end(); UI != UE;) {
Yay for use_iterators :)
>
> + MachineOperand &UseMO = UI.getOperand();
> + ++UI;
> + MachineInstr *UseMI = UseMO.getParent();
It would be more clear to use (before the increment):
MachineInstr *UseMI = *UI;
Note that if an instruction uses a register multiple times,
incrementing the iterator could point into another operand of the same
instruction. This is rare, but the code needs to handle it
correctly. I'm concerned about cases like:
r1 = or r2, r2
(which is the copy operation on some targets).
Where the use iterator could point to the first r2, and incrementing
it could point to the next r2. This could be avoided by not zapping
instructions in this loop: move the copy zapping to a walk over a
smallset or something.
> + unsigned UseIdx = li_->getInstructionIndex(UseMI);
> + LiveInterval::iterator ULR =
> IntA.FindLiveRangeContaining(UseIdx);
> + if (ULR->valno != AValNo)
> + continue;
> + UseMO.setReg(NewReg);
> + if (UseMO.isKill())
> + BKills.push_back(li_->getUseIndex(UseIdx)+1);
> + if (UseMI != CopyMI) {
if (UseMI == CopyMI) continue;
>
> + unsigned SrcReg, DstReg;
> + if (!tii_->isMoveInstr(*UseMI, SrcReg, DstReg))
> + continue;
> + unsigned repDstReg = rep(DstReg);
> + if (repDstReg != IntB.reg) {
> + // Update dst register interval val# since its source
> register has
> + // changed.
> + LiveInterval &DLI = li_->getInterval(repDstReg);
> + LiveInterval::iterator DLR =
> + DLI.FindLiveRangeContaining(li_->getDefIndex(UseIdx));
> + DLR->valno->reg = NewReg;
> + ChangedCopies.insert(UseMI);
> + } else {
> + // This copy will become a noop. If it's defining a new val#,
> + // remove that val# as well. However this live range is being
> + // extended to the end of the existing live range defined
> by the copy.
> + unsigned DefIdx = li_->getDefIndex(UseIdx);
> + LiveInterval::iterator DLR =
> IntB.FindLiveRangeContaining(DefIdx);
> + BHasPHIKill |= DLR->valno->hasPHIKill;
> + assert(DLR->valno->def == DefIdx);
> + BDeadValNos.push_back(DLR->valno);
> + BExtend[DLR->start] = DLR->end;
> + JoinedCopies.insert(UseMI);
> + // If this is a kill but it's going to be removed, the last
> use
> + // of the same val# is the new kill.
> + if (UseMO.isKill()) {
> + BKills.pop_back();
> + }
> + }
It isn't clear to me what this code is doing. It looks like it is
looking for the copy that has now becomes an identity copy. Is there
some way to factor this with other code that is coallescing copies
away? It seems duplicated from somewhere else. It would be nicer to
just add all copies to a small vector and then zap them later or
something, to keep the logic simple.
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SimpleRegisterCoalescing.h (original)
> +++ llvm/trunk/lib/CodeGen/SimpleRegisterCoalescing.h Tue Feb 12
> 21:01:43 2008
> @@ -80,6 +80,7 @@
>
> + /// ChangedCopies - Keep track of copies modified due to
> commuting.
> + SmallPtrSet<MachineInstr*, 32> ChangedCopies;
Does this need to be an ivar? Can it just be passed by-ref between
the methods that need it?
-Chris
More information about the llvm-commits
mailing list