[llvm] r276670 - Revert NewGVN N^2 behavior patch

[David Blaikie via llvm-commits]

On Mon, Jul 25, 2016 at 11:27 AM Daniel Berlin via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: dannyb
> Date: Mon Jul 25 13:19:49 2016
> New Revision: 276670
>
> URL: http://llvm.org/viewvc/llvm-project?rev=276670&view=rev
> Log:
> Revert NewGVN N^2 behavior patch
>

Helpful to mention why a patch is being reverted (not clear - but I'm
guessing/assuming this was the patch to /fix/ the N^2 behavior? Or was it a
patch that (accidentally) introduced N^2 behavior? (what was the patch
intended to do, then?)) & the revision number in the commit message,
ideally.


>
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=276670&r1=276669&r2=276670&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Mon Jul 25 13:19:49 2016
> @@ -53,10 +53,10 @@ namespace {
>  // Provides a sorting function based on the execution order of two
> instructions.
>  struct SortByDFSIn {
>  private:
> -  DenseMap<const Value *, unsigned> &DFSNumber;
> +  DenseMap<const BasicBlock *, unsigned> &DFSNumber;
>
>  public:
> -  SortByDFSIn(DenseMap<const Value *, unsigned> &D) : DFSNumber(D) {}
> +  SortByDFSIn(DenseMap<const BasicBlock *, unsigned> &D) : DFSNumber(D) {}
>
>    // Returns true when A executes before B.
>    bool operator()(const Instruction *A, const Instruction *B) const {
> @@ -74,10 +74,9 @@ public:
>      if (NA < NB)
>        return true;
>      if (NA == NB) {
> -      unsigned ADFS = DFSNumber.lookup(A);
> -      unsigned BDFS = DFSNumber.lookup(B);
> -      assert (ADFS && ADFS);
> -      return ADFS < BDFS;
> +      // Sort them in the order they occur in the same basic block.
> +      BasicBlock::const_iterator AI(A), BI(B);
> +      return std::distance(AI, BI) < 0;
>      }
>      return false;
>    }
> @@ -197,13 +196,9 @@ public:
>      VN.setMemDep(MD);
>      bool Res = false;
>
> -    // Perform DFS Numbering of blocks and instructions.
>      unsigned I = 0;
> -    for (const BasicBlock *BB : depth_first(&F.getEntryBlock())) {
> +    for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
>        DFSNumber.insert({BB, ++I});
> -      for (auto &Inst: *BB)
> -        DFSNumber.insert({&Inst, ++I});
> -    }
>
>      // FIXME: use lazy evaluation of VN to avoid the fix-point
> computation.
>      while (1) {
> @@ -233,7 +228,7 @@ private:
>    AliasAnalysis *AA;
>    MemoryDependenceResults *MD;
>    const bool OptForMinSize;
> -  DenseMap<const Value *, unsigned> DFSNumber;
> +  DenseMap<const BasicBlock *, unsigned> DFSNumber;
>    BBSideEffectsSet BBSideEffects;
>    MemorySSA *MSSA;
>    int HoistedCtr;
> @@ -295,14 +290,16 @@ private:
>    }
>
>    /* Return true when I1 appears before I2 in the instructions of BB.  */
> -  bool firstInBB(const Instruction *I1, const Instruction *I2) {
> -    assert (I1->getParent() == I2->getParent());
> -    unsigned I1DFS = DFSNumber.lookup(I1);
> -    unsigned I2DFS = DFSNumber.lookup(I2);
> -    assert (I1DFS && I2DFS);
> -    return I1DFS < I2DFS;
> -  }
> +  bool firstInBB(BasicBlock *BB, const Instruction *I1, const Instruction
> *I2) {
> +    for (Instruction &I : *BB) {
> +      if (&I == I1)
> +        return true;
> +      if (&I == I2)
> +        return false;
> +    }
>
> +    llvm_unreachable("I1 and I2 not found in BB");
> +  }
>    // Return true when there are users of Def in BB.
>    bool hasMemoryUseOnPath(MemoryAccess *Def, const BasicBlock *BB,
>                            const Instruction *OldPt) {
> @@ -326,7 +323,7 @@ private:
>            return true;
>
>          // It is only harmful to hoist when the use is before OldPt.
> -        if (firstInBB(MU->getMemoryInst(), OldPt))
> +        if (firstInBB(UBB, MU->getMemoryInst(), OldPt))
>            return true;
>        }
>
> @@ -440,7 +437,7 @@ private:
>
>      if (NewBB == DBB && !MSSA->isLiveOnEntryDef(D))
>        if (auto *UD = dyn_cast<MemoryUseOrDef>(D))
> -        if (firstInBB(NewPt, UD->getMemoryInst()))
> +        if (firstInBB(DBB, NewPt, UD->getMemoryInst()))
>            // Cannot move the load or store to NewPt above its definition
> in D.
>            return false;
>
> @@ -517,7 +514,7 @@ private:
>
>        if (BB == HoistBB) {
>          NewHoistBB = HoistBB;
> -        NewHoistPt = firstInBB(Insn, HoistPt) ? Insn : HoistPt;
> +        NewHoistPt = firstInBB(BB, Insn, HoistPt) ? Insn : HoistPt;
>        } else {
>          NewHoistBB = DT->findNearestCommonDominator(HoistBB, BB);
>          if (NewHoistBB == BB)
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160801/1d95e849/attachment.html>