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.<div>
Thanks Jakob for working on this!</div><div><br></div><div>John<br><br><div class="gmail_quote">On Tue, Aug 11, 2009 at 1:20 AM, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com">evan.cheng@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div><div></div><div class="h5"><br>
On Aug 10, 2009, at 11:25 PM, Jakob Stoklund Olesen wrote:<br>
<br>
> Author: stoklund<br>
> Date: Tue Aug 11 01:25:12 2009<br>
> New Revision: 78650<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=78650&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=78650&view=rev</a><br>
> Log:<br>
> Rebuild RegScavenger::DistanceMap each time it is needed.<br>
><br>
> The register scavenger maintains a DistanceMap that maps MI pointers<br>
> to their<br>
> distance from the top of the current MBB. The DistanceMap is built<br>
> incrementally in forward() and in bulk in findFirstUse(). It is used<br>
> by<br>
> scavengeRegister() to determine which candidate register has the<br>
> longest<br>
> unused interval.<br>
><br>
> Unfortunately the DistanceMap contents can become outdated. The<br>
> first time<br>
> scavengeRegister() is called, the DistanceMap is filled to cover the<br>
> MBB. If<br>
> then instructions are inserted in the MBB (as they always are<br>
> following<br>
> scavengeRegister()), the recorded distances are too short. This<br>
> causes bad<br>
> behaviour in the included test case where a register use /after/ the<br>
> current<br>
> position is ignored because findFirstUse() thinks is is /before/ the<br>
> current<br>
> position. A "using an undefined register" assertion follows promptly.<br>
<br>
</div></div>Thanks Jakob. Since you are hacking on this, perhaps you can help me<br>
clean it up further? :-) I think we need to completely do away with<br>
DistanceMap. I think using it is a very bad idea.<br>
<br>
Or rather findFirstUse() is a bad idea. It's using MachineRegisterInfo<br>
to iterate over uses of *physical* registers. That's incredibly<br>
expensive since the number of uses in a large function can be very big.<br>
<br>
I think a better solution is perhaps something like this. First create<br>
a set of all candidates. Then iterate over the next N instructions<br>
(where N is reasonable number of instructions, perhaps something like<br>
10?) and calculate the distance of the closest use (and use of any<br>
alias). This is not a great solution but it would avoid the really bad<br>
pathological case.<br>
<div class="im"><br>
><br>
> The fix is to build a fresh DistanceMap at the top of<br>
> scavengeRegister(), and<br>
> discard it after use. This means that DistanceMap is no longer<br>
> needed as a<br>
> RegScavenger member variable, and forward() doesn't need to update it.<br>
><br>
> The fix then discloses issue number two in the same test case: The<br>
> candidate<br>
> search in scavengeRegister() finds a CSR that has been saved in the<br>
> prologue,<br>
> but is currently unused. It would be both inefficient and wrong to<br>
> spill such<br>
> a register in the emergency spill slot. In the present case, the<br>
> emergency<br>
> slot restore is placed immediately before the normal epilogue<br>
> restore, leading<br>
> to a "Redefining a live register" assertion.<br>
><br>
> Fix number two: When scavengerRegister() stumbles upon an unused<br>
> register that<br>
> is overwritten later in the MBB, return that register early. It is<br>
> important<br>
> to verify that the register is defined later in the MBB, otherwise<br>
> it might be<br>
> an unspilled CSR.<br>
<br>
</div>Thanks for fixing this.<br>
<font color="#888888"><br>
Evan<br>
</font><div><div></div><div class="h5"><br>
><br>
> Added:<br>
>    llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll<br>
> Modified:<br>
>    llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h<br>
>    llvm/trunk/lib/CodeGen/RegisterScavenging.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h?rev=78650&r1=78649&r2=78650&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h?rev=78650&r1=78649&r2=78650&view=diff</a><br>

><br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> ======================================================================<br>
> --- llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h (original)<br>
> +++ llvm/trunk/include/llvm/CodeGen/RegisterScavenging.h Tue Aug 11<br>
> 01:25:12 2009<br>
> @@ -19,7 +19,6 @@<br>
><br>
> #include "llvm/CodeGen/MachineBasicBlock.h"<br>
> #include "llvm/ADT/BitVector.h"<br>
> -#include "llvm/ADT/DenseMap.h"<br>
><br>
> namespace llvm {<br>
><br>
> @@ -69,14 +68,6 @@<br>
>   /// available, unset means the register is currently being used.<br>
>   BitVector RegsAvailable;<br>
><br>
> -  /// CurrDist - Distance from MBB entry to the current instruction<br>
> MBBI.<br>
> -  ///<br>
> -  unsigned CurrDist;<br>
> -<br>
> -  /// DistanceMap - Keep track the distance of a MI from the start<br>
> of the<br>
> -  /// current basic block.<br>
> -  DenseMap<MachineInstr*, unsigned> DistanceMap;<br>
> -<br>
> public:<br>
>   RegScavenger()<br>
>     : MBB(NULL), NumPhysRegs(0), Tracking(false),<br>
> @@ -112,6 +103,9 @@<br>
>   bool isUsed(unsigned Reg) const   { return !RegsAvailable[Reg]; }<br>
>   bool isUnused(unsigned Reg) const { return RegsAvailable[Reg]; }<br>
><br>
> +  /// isAliasUsed - Is Reg or an alias currently in use?<br>
> +  bool isAliasUsed(unsigned Reg) const;<br>
> +<br>
>   /// getRegsUsed - return all registers currently in use in used.<br>
>   void getRegsUsed(BitVector &used, bool includeReserved);<br>
><br>
> @@ -156,10 +150,6 @@<br>
>   /// emergency spill slot. Mark it used.<br>
>   void restoreScavengedReg();<br>
><br>
> -  MachineInstr *findFirstUse(MachineBasicBlock *MBB,<br>
> -                             MachineBasicBlock::iterator I,<br>
> unsigned Reg,<br>
> -                             unsigned &Dist);<br>
> -<br>
>   /// Add Reg and all its sub-registers to BV.<br>
>   void addRegWithSubRegs(BitVector &BV, unsigned Reg);<br>
><br>
><br>
> Modified: llvm/trunk/lib/CodeGen/RegisterScavenging.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegisterScavenging.cpp?rev=78650&r1=78649&r2=78650&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegisterScavenging.cpp?rev=78650&r1=78649&r2=78650&view=diff</a><br>

><br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> ======================================================================<br>
> --- llvm/trunk/lib/CodeGen/RegisterScavenging.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/RegisterScavenging.cpp Tue Aug 11<br>
> 01:25:12 2009<br>
> @@ -25,6 +25,7 @@<br>
> #include "llvm/Target/TargetRegisterInfo.h"<br>
> #include "llvm/Target/TargetInstrInfo.h"<br>
> #include "llvm/Target/TargetMachine.h"<br>
> +#include "llvm/ADT/DenseMap.h"<br>
> #include "llvm/ADT/SmallPtrSet.h"<br>
> #include "llvm/ADT/SmallVector.h"<br>
> #include "llvm/ADT/STLExtras.h"<br>
> @@ -48,12 +49,19 @@<br>
>     RegsAvailable.set(SubReg);<br>
> }<br>
><br>
> +bool RegScavenger::isAliasUsed(unsigned Reg) const {<br>
> +  if (isUsed(Reg))<br>
> +    return true;<br>
> +  for (const unsigned *R = TRI->getAliasSet(Reg); *R; ++R)<br>
> +    if (isUsed(*R))<br>
> +      return true;<br>
> +  return false;<br>
> +}<br>
> +<br>
> void RegScavenger::initRegState() {<br>
>   ScavengedReg = 0;<br>
>   ScavengedRC = NULL;<br>
>   ScavengeRestore = NULL;<br>
> -  CurrDist = 0;<br>
> -  DistanceMap.clear();<br>
><br>
>   // All registers started out unused.<br>
>   RegsAvailable.set();<br>
> @@ -176,7 +184,6 @@<br>
>   }<br>
><br>
>   MachineInstr *MI = MBBI;<br>
> -  DistanceMap.insert(std::make_pair(MI, CurrDist++));<br>
><br>
>   if (MI == ScavengeRestore) {<br>
>     ScavengedReg = 0;<br>
> @@ -286,12 +293,25 @@<br>
>   return (Reg == -1) ? 0 : Reg;<br>
> }<br>
><br>
> +/// DistanceMap - Keep track the distance of an MI from the current<br>
> position.<br>
> +typedef DenseMap<MachineInstr*, unsigned> DistanceMap;<br>
> +<br>
> +/// Build a distance map for instructions from I to E.<br>
> +static void buildDistanceMap(DistanceMap &DM,<br>
> +                             MachineBasicBlock::iterator I,<br>
> +                             MachineBasicBlock::iterator E) {<br>
> +  DM.clear();<br>
> +  for (unsigned d = 0; I != E; ++I, ++d)<br>
> +    DM.insert(DistanceMap::value_type(I, d));<br>
> +}<br>
> +<br>
> /// findFirstUse - Calculate the distance to the first use of the<br>
> -/// specified register.<br>
> -MachineInstr*<br>
> -RegScavenger::findFirstUse(MachineBasicBlock *MBB,<br>
> -                           MachineBasicBlock::iterator I, unsigned<br>
> Reg,<br>
> -                           unsigned &Dist) {<br>
> +/// specified register in the range covered by DM.<br>
> +static MachineInstr *findFirstUse(const MachineBasicBlock *MBB,<br>
> +                                  const DistanceMap &DM,<br>
> +                                  unsigned Reg,<br>
> +                                  unsigned &Dist) {<br>
> +  const MachineRegisterInfo *MRI = &MBB->getParent()->getRegInfo();<br>
>   MachineInstr *UseMI = 0;<br>
>   Dist = ~0U;<br>
>   for (MachineRegisterInfo::reg_iterator RI = MRI->reg_begin(Reg),<br>
> @@ -299,19 +319,10 @@<br>
>     MachineInstr *UDMI = &*RI;<br>
>     if (UDMI->getParent() != MBB)<br>
>       continue;<br>
> -    DenseMap<MachineInstr*, unsigned>::iterator DI =<br>
> DistanceMap.find(UDMI);<br>
> -    if (DI == DistanceMap.end()) {<br>
> -      // If it's not in map, it's below current MI, let's<br>
> initialize the<br>
> -      // map.<br>
> -      I = next(I);<br>
> -      unsigned Dist = CurrDist + 1;<br>
> -      while (I != MBB->end()) {<br>
> -        DistanceMap.insert(std::make_pair(I, Dist++));<br>
> -        I = next(I);<br>
> -      }<br>
> -    }<br>
> -    DI = DistanceMap.find(UDMI);<br>
> -    if (DI->second > CurrDist && DI->second < Dist) {<br>
> +    DistanceMap::const_iterator DI = DM.find(UDMI);<br>
> +    if (DI == DM.end())<br>
> +      continue;<br>
> +    if (DI->second < Dist) {<br>
>       Dist = DI->second;<br>
>       UseMI = UDMI;<br>
>     }<br>
> @@ -338,6 +349,10 @@<br>
>       Candidates.reset(MO.getReg());<br>
>   }<br>
><br>
> +  // Prepare to call findFirstUse() a number of times.<br>
> +  DistanceMap DM;<br>
> +  buildDistanceMap(DM, I, MBB->end());<br>
> +<br>
>   // Find the register whose use is furthest away.<br>
>   unsigned SReg = 0;<br>
>   unsigned MaxDist = 0;<br>
> @@ -345,15 +360,23 @@<br>
>   int Reg = Candidates.find_first();<br>
>   while (Reg != -1) {<br>
>     unsigned Dist;<br>
> -    MachineInstr *UseMI = findFirstUse(MBB, I, Reg, Dist);<br>
> +    MachineInstr *UseMI = findFirstUse(MBB, DM, Reg, Dist);<br>
>     for (const unsigned *AS = TRI->getAliasSet(Reg); *AS; ++AS) {<br>
>       unsigned AsDist;<br>
> -      MachineInstr *AsUseMI = findFirstUse(MBB, I, *AS, AsDist);<br>
> +      MachineInstr *AsUseMI = findFirstUse(MBB, DM, *AS, AsDist);<br>
>       if (AsDist < Dist) {<br>
>         Dist = AsDist;<br>
>         UseMI = AsUseMI;<br>
>       }<br>
>     }<br>
> +<br>
> +    // If we found an unused register that is defined by a later<br>
> instruction,<br>
> +    // there is no reason to spill it. We have probably found a<br>
> callee-saved<br>
> +    // register that has been saved in the prologue, but happens to<br>
> be unused at<br>
> +    // this point.<br>
> +    if (!isAliasUsed(Reg) && UseMI != NULL)<br>
> +      return Reg;<br>
> +<br>
>     if (Dist >= MaxDist) {<br>
>       MaxDist = Dist;<br>
>       MaxUseMI = UseMI;<br>
><br>
> Added: llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll?rev=78650&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll?rev=78650&view=auto</a><br>

><br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> =<br>
> ======================================================================<br>
> --- llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll<br>
> (added)<br>
> +++ llvm/trunk/test/CodeGen/Blackfin/2009-08-11-RegScavenger-CSR.ll<br>
> Tue Aug 11 01:25:12 2009<br>
> @@ -0,0 +1,17 @@<br>
> +; RUN: llvm-as < %s | llc -march=bfin -verify-machineinstrs<br>
> +<br>
> +declare i64 @llvm.cttz.i64(i64) nounwind readnone<br>
> +<br>
> +declare i16 @llvm.cttz.i16(i16) nounwind readnone<br>
> +<br>
> +declare i8 @llvm.cttz.i8(i8) nounwind readnone<br>
> +<br>
> +define void @cttztest(i8 %A, i16 %B, i32 %C, i64 %D, i8* %AP, i16*<br>
> %BP, i32* %CP, i64* %DP) {<br>
> +     %a = call i8 @llvm.cttz.i8(i8 %A)               ; <i8> [#uses=1]<br>
> +     %b = call i16 @llvm.cttz.i16(i16 %B)            ; <i16> [#uses=1]<br>
> +     %d = call i64 @llvm.cttz.i64(i64 %D)            ; <i64> [#uses=1]<br>
> +     store i8 %a, i8* %AP<br>
> +     store i16 %b, i16* %BP<br>
> +     store i64 %d, i64* %DP<br>
> +     ret void<br>
> +}<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>