[llvm-commits] [llvm] r78650 - in /llvm/trunk: include/llvm/CodeGen/RegisterScavenging.h lib/CodeGen/RegisterScavenging.cpp test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll
John Mosby
ojomojo at gmail.com
Tue Aug 11 10:02:26 PDT 2009
Thanks Evan, that's a great idea. I have seen this behavior on some of my
contrived test cases that contain huge inlined functions. I am starting to
use RS inside PEI (in my sandbox) and have only run into this in one or two
admittedly contrived test cases, but it's clearly a problem.Thanks Jakob for
working on this!
John
On Tue, Aug 11, 2009 at 1:20 AM, Evan Cheng <evan.cheng at apple.com> wrote:
>
> On Aug 10, 2009, at 11:25 PM, Jakob Stoklund Olesen wrote:
>
> > Author: stoklund
> > Date: Tue Aug 11 01:25:12 2009
> > New Revision: 78650
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=78650&view=rev
> > Log:
> > Rebuild RegScavenger::DistanceMap each time it is needed.
> >
> > The register scavenger maintains a DistanceMap that maps MI pointers
> > to their
> > distance from the top of the current MBB. The DistanceMap is built
> > incrementally in forward() and in bulk in findFirstUse(). It is used
> > by
> > scavengeRegister() to determine which candidate register has the
> > longest
> > unused interval.
> >
> > Unfortunately the DistanceMap contents can become outdated. The
> > first time
> > scavengeRegister() is called, the DistanceMap is filled to cover the
> > MBB. If
> > then instructions are inserted in the MBB (as they always are
> > following
> > scavengeRegister()), the recorded distances are too short. This
> > causes bad
> > behaviour in the included test case where a register use /after/ the
> > current
> > position is ignored because findFirstUse() thinks is is /before/ the
> > current
> > position. A "using an undefined register" assertion follows promptly.
>
> Thanks Jakob. Since you are hacking on this, perhaps you can help me
> clean it up further? :-) I think we need to completely do away with
> DistanceMap. I think using it is a very bad idea.
>
> Or rather findFirstUse() is a bad idea. It's using MachineRegisterInfo
> to iterate over uses of *physical* registers. That's incredibly
> expensive since the number of uses in a large function can be very big.
>
> I think a better solution is perhaps something like this. First create
> a set of all candidates. Then iterate over the next N instructions
> (where N is reasonable number of instructions, perhaps something like
> 10?) and calculate the distance of the closest use (and use of any
> alias). This is not a great solution but it would avoid the really bad
> pathological case.
>
> >
> > The fix is to build a fresh DistanceMap at the top of
> > scavengeRegister(), and
> > discard it after use. This means that DistanceMap is no longer
> > needed as a
> > RegScavenger member variable, and forward() doesn't need to update it.
> >
> > The fix then discloses issue number two in the same test case: The
> > candidate
> > search in scavengeRegister() finds a CSR that has been saved in the
> > prologue,
> > but is currently unused. It would be both inefficient and wrong to
> > spill such
> > a register in the emergency spill slot. In the present case, the
> > emergency
> > slot restore is placed immediately before the normal epilogue
> > restore, leading
> > to a "Redefining a live register" assertion.
> >
> > Fix number two: When scavengerRegister() stumbles upon an unused
> > register that
> > is overwritten later in the MBB, return that register early. It is
> > important
> > to verify that the register is defined later in the MBB, otherwise
> > it might be
> > an unspilled CSR.
>
> Thanks for fixing this.
>
> Evan
>
> >
> > Added:
> > llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll
> > Modified:
> > llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h
> > llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
> >
> > Modified: llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h?rev=78650&r1=78649&r2=78650&view=diff
> >
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > ======================================================================
> > --- llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h (original)
> > +++ llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h Tue Aug 11
> > 01:25:12 2009
> > @@ -19,7 +19,6 @@
> >
> > #include "llvm/CodeGen/MachineBasicBlock.h"
> > #include "llvm/ADT/BitVector.h"
> > -#include "llvm/ADT/DenseMap.h"
> >
> > namespace llvm {
> >
> > @@ -69,14 +68,6 @@
> > /// available, unset means the register is currently being used.
> > BitVector RegsAvailable;
> >
> > - /// CurrDist - Distance from MBB entry to the current instruction
> > MBBI.
> > - ///
> > - unsigned CurrDist;
> > -
> > - /// DistanceMap - Keep track the distance of a MI from the start
> > of the
> > - /// current basic block.
> > - DenseMap<MachineInstr*, unsigned> DistanceMap;
> > -
> > public:
> > RegScavenger()
> > : MBB(NULL), NumPhysRegs(0), Tracking(false),
> > @@ -112,6 +103,9 @@
> > bool isUsed(unsigned Reg) const { return !RegsAvailable[Reg]; }
> > bool isUnused(unsigned Reg) const { return RegsAvailable[Reg]; }
> >
> > + /// isAliasUsed - Is Reg or an alias currently in use?
> > + bool isAliasUsed(unsigned Reg) const;
> > +
> > /// getRegsUsed - return all registers currently in use in used.
> > void getRegsUsed(BitVector &used, bool includeReserved);
> >
> > @@ -156,10 +150,6 @@
> > /// emergency spill slot. Mark it used.
> > void restoreScavengedReg();
> >
> > - MachineInstr *findFirstUse(MachineBasicBlock *MBB,
> > - MachineBasicBlock::iterator I,
> > unsigned Reg,
> > - unsigned &Dist);
> > -
> > /// Add Reg and all its sub-registers to BV.
> > void addRegWithSubRegs(BitVector &BV, unsigned Reg);
> >
> >
> > Modified: llvm/trunk/lib/CodeGen/RegisterScavenging.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegisterScavenging.cpp?rev=78650&r1=78649&r2=78650&view=diff
> >
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > ======================================================================
> > --- llvm/trunk/lib/CodeGen/RegisterScavenging.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/RegisterScavenging.cpp Tue Aug 11
> > 01:25:12 2009
> > @@ -25,6 +25,7 @@
> > #include "llvm/Target/TargetRegisterInfo.h"
> > #include "llvm/Target/TargetInstrInfo.h"
> > #include "llvm/Target/TargetMachine.h"
> > +#include "llvm/ADT/DenseMap.h"
> > #include "llvm/ADT/SmallPtrSet.h"
> > #include "llvm/ADT/SmallVector.h"
> > #include "llvm/ADT/STLExtras.h"
> > @@ -48,12 +49,19 @@
> > RegsAvailable.set(SubReg);
> > }
> >
> > +bool RegScavenger::isAliasUsed(unsigned Reg) const {
> > + if (isUsed(Reg))
> > + return true;
> > + for (const unsigned *R = TRI->getAliasSet(Reg); *R; ++R)
> > + if (isUsed(*R))
> > + return true;
> > + return false;
> > +}
> > +
> > void RegScavenger::initRegState() {
> > ScavengedReg = 0;
> > ScavengedRC = NULL;
> > ScavengeRestore = NULL;
> > - CurrDist = 0;
> > - DistanceMap.clear();
> >
> > // All registers started out unused.
> > RegsAvailable.set();
> > @@ -176,7 +184,6 @@
> > }
> >
> > MachineInstr *MI = MBBI;
> > - DistanceMap.insert(std::make_pair(MI, CurrDist++));
> >
> > if (MI == ScavengeRestore) {
> > ScavengedReg = 0;
> > @@ -286,12 +293,25 @@
> > return (Reg == -1) ? 0 : Reg;
> > }
> >
> > +/// DistanceMap - Keep track the distance of an MI from the current
> > position.
> > +typedef DenseMap<MachineInstr*, unsigned> DistanceMap;
> > +
> > +/// Build a distance map for instructions from I to E.
> > +static void buildDistanceMap(DistanceMap &DM,
> > + MachineBasicBlock::iterator I,
> > + MachineBasicBlock::iterator E) {
> > + DM.clear();
> > + for (unsigned d = 0; I != E; ++I, ++d)
> > + DM.insert(DistanceMap::value_type(I, d));
> > +}
> > +
> > /// findFirstUse - Calculate the distance to the first use of the
> > -/// specified register.
> > -MachineInstr*
> > -RegScavenger::findFirstUse(MachineBasicBlock *MBB,
> > - MachineBasicBlock::iterator I, unsigned
> > Reg,
> > - unsigned &Dist) {
> > +/// specified register in the range covered by DM.
> > +static MachineInstr *findFirstUse(const MachineBasicBlock *MBB,
> > + const DistanceMap &DM,
> > + unsigned Reg,
> > + unsigned &Dist) {
> > + const MachineRegisterInfo *MRI = &MBB->getParent()->getRegInfo();
> > MachineInstr *UseMI = 0;
> > Dist = ~0U;
> > for (MachineRegisterInfo::reg_iterator RI = MRI->reg_begin(Reg),
> > @@ -299,19 +319,10 @@
> > MachineInstr *UDMI = &*RI;
> > if (UDMI->getParent() != MBB)
> > continue;
> > - DenseMap<MachineInstr*, unsigned>::iterator DI =
> > DistanceMap.find(UDMI);
> > - if (DI == DistanceMap.end()) {
> > - // If it's not in map, it's below current MI, let's
> > initialize the
> > - // map.
> > - I = next(I);
> > - unsigned Dist = CurrDist + 1;
> > - while (I != MBB->end()) {
> > - DistanceMap.insert(std::make_pair(I, Dist++));
> > - I = next(I);
> > - }
> > - }
> > - DI = DistanceMap.find(UDMI);
> > - if (DI->second > CurrDist && DI->second < Dist) {
> > + DistanceMap::const_iterator DI = DM.find(UDMI);
> > + if (DI == DM.end())
> > + continue;
> > + if (DI->second < Dist) {
> > Dist = DI->second;
> > UseMI = UDMI;
> > }
> > @@ -338,6 +349,10 @@
> > Candidates.reset(MO.getReg());
> > }
> >
> > + // Prepare to call findFirstUse() a number of times.
> > + DistanceMap DM;
> > + buildDistanceMap(DM, I, MBB->end());
> > +
> > // Find the register whose use is furthest away.
> > unsigned SReg = 0;
> > unsigned MaxDist = 0;
> > @@ -345,15 +360,23 @@
> > int Reg = Candidates.find_first();
> > while (Reg != -1) {
> > unsigned Dist;
> > - MachineInstr *UseMI = findFirstUse(MBB, I, Reg, Dist);
> > + MachineInstr *UseMI = findFirstUse(MBB, DM, Reg, Dist);
> > for (const unsigned *AS = TRI->getAliasSet(Reg); *AS; ++AS) {
> > unsigned AsDist;
> > - MachineInstr *AsUseMI = findFirstUse(MBB, I, *AS, AsDist);
> > + MachineInstr *AsUseMI = findFirstUse(MBB, DM, *AS, AsDist);
> > if (AsDist < Dist) {
> > Dist = AsDist;
> > UseMI = AsUseMI;
> > }
> > }
> > +
> > + // If we found an unused register that is defined by a later
> > instruction,
> > + // there is no reason to spill it. We have probably found a
> > callee-saved
> > + // register that has been saved in the prologue, but happens to
> > be unused at
> > + // this point.
> > + if (!isAliasUsed(Reg) && UseMI != NULL)
> > + return Reg;
> > +
> > if (Dist >= MaxDist) {
> > MaxDist = Dist;
> > MaxUseMI = UseMI;
> >
> > Added: llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll?rev=78650&view=auto
> >
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > =
> > ======================================================================
> > --- llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll
> > (added)
> > +++ llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll
> > Tue Aug 11 01:25:12 2009
> > @@ -0,0 +1,17 @@
> > +; RUN: llvm-as < %s | llc -march=bfin -verify-machineinstrs
> > +
> > +declare i64 @llvm.cttz.i64(i64) nounwind readnone
> > +
> > +declare i16 @llvm.cttz.i16(i16) nounwind readnone
> > +
> > +declare i8 @llvm.cttz.i8(i8) nounwind readnone
> > +
> > +define void @cttztest(i8 %A, i16 %B, i32 %C, i64 %D, i8* %AP, i16*
> > %BP, i32* %CP, i64* %DP) {
> > + %a = call i8 @llvm.cttz.i8(i8 %A) ; <i8> [#uses=1]
> > + %b = call i16 @llvm.cttz.i16(i16 %B) ; <i16> [#uses=1]
> > + %d = call i64 @llvm.cttz.i64(i64 %D) ; <i64> [#uses=1]
> > + store i8 %a, i8* %AP
> > + store i16 %b, i16* %BP
> > + store i64 %d, i64* %DP
> > + ret void
> > +}
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090811/3f0b36f1/attachment.html>
More information about the llvm-commits
mailing list