[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
Evan Cheng
evan.cheng at apple.com
Tue Aug 11 00:20:04 PDT 2009
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
More information about the llvm-commits
mailing list