[PATCH] please review, non-determinism bug fix in gvn

[Benjamin Kramer]

On 13.05.2014, at 00:22, Daniel Reynaud <dreynaud at apple.com> wrote:

> 
> On May 12, 2014, at 11:00 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
> 
>> Nice catch! I'm a bit afraid of adding an erase() member to MapVector as erasing from a vector is expensive. It looks like you can easily avoid the erasing in this case by not adding edges that are going to be split to the map at all (and assert that they're not in the map when doing the actual splitting). Sounds reasonable?
> 
> Good point, here goes:
> 
> 
> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp
> index 61f3f27..3186dfe 100644
> --- a/lib/Transforms/Scalar/GVN.cpp
> +++ b/lib/Transforms/Scalar/GVN.cpp
> @@ -20,6 +20,7 @@
> #include "llvm/ADT/DenseMap.h"
> #include "llvm/ADT/DepthFirstIterator.h"
> #include "llvm/ADT/Hashing.h"
> +#include "llvm/ADT/MapVector.h"
> #include "llvm/ADT/SetVector.h"
> #include "llvm/ADT/SmallPtrSet.h"
> #include "llvm/ADT/Statistic.h"
> @@ -1539,7 +1540,7 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
> 
>   // Check to see how many predecessors have the loaded value fully
>   // available.
> -  DenseMap<BasicBlock*, Value*> PredLoads;
> +  MapVector<BasicBlock*, Value*> PredLoads;
>   DenseMap<BasicBlock*, char> FullyAvailableBlocks;
>   for (unsigned i = 0, e = ValuesPerBlock.size(); i != e; ++i)
>     FullyAvailableBlocks[ValuesPerBlock[i].BB] = true;
> @@ -1553,7 +1554,6 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>     if (IsValueFullyAvailableInBlock(Pred, FullyAvailableBlocks, 0)) {
>       continue;
>     }
> -    PredLoads[Pred] = 0;
> 
>     if (Pred->getTerminator()->getNumSuccessors() != 1) {
>       if (isa<IndirectBrInst>(Pred->getTerminator())) {
> @@ -1570,11 +1570,14 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>       }
> 
>       CriticalEdgePred.push_back(Pred);
> +    } else {
> +      // only add the predecessors that will not be split for now
> +      PredLoads[Pred] = 0;
>     }
>   }
> 
>   // Decide whether PRE is profitable for this load.
> -  unsigned NumUnavailablePreds = PredLoads.size();
> +  unsigned NumUnavailablePreds = PredLoads.size() + CriticalEdgePred.size();
>   assert(NumUnavailablePreds != 0 &&
>          "Fully available value should already be eliminated!");
> 
> @@ -1590,7 +1593,6 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>          E = CriticalEdgePred.end(); I != E; I++) {
>     BasicBlock *OrigPred = *I;
>     BasicBlock *NewPred = splitCriticalEdges(OrigPred, LoadBB);
> -    PredLoads.erase(OrigPred);
>     PredLoads[NewPred] = 0;
>     DEBUG(dbgs() << "Split critical edge " << OrigPred->getName() << "->"
>                  << LoadBB->getName() << '\n');
> @@ -1599,7 +1601,7 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>   // Check if the load can safely be moved to all the unavailable predecessors.
>   bool CanDoPRE = true;
>   SmallVector<Instruction*, 8> NewInsts;
> -  for (DenseMap<BasicBlock*, Value*>::iterator I = PredLoads.begin(),
> +  for (MapVector<BasicBlock*, Value*>::iterator I = PredLoads.begin(),
>          E = PredLoads.end(); I != E; ++I) {
>     BasicBlock *UnavailablePred = I->first;
> 
> @@ -1633,7 +1635,7 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>       I->eraseFromParent();
>     }
>     // HINT:Don't revert the edge-splitting as following transformation may 
> -    // also need to split these critial edges.
> +    // also need to split these critical edges.
>     return !CriticalEdgePred.empty();
>   }
> 
> @@ -1654,7 +1656,7 @@ bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
>     VN.lookup_or_add(NewInsts[i]);
>   }
> 
> -  for (DenseMap<BasicBlock*, Value*>::iterator I = PredLoads.begin(),
> +  for (MapVector<BasicBlock*, Value*>::iterator I = PredLoads.begin(),
>          E = PredLoads.end(); I != E; ++I) {
>     BasicBlock *UnavailablePred = I->first;
>     Value *LoadPtr = I->second;

Looks good. Tweaked it a bit to apply against trunk and applied in r208728.

- Ben