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

[Daniel Berlin via llvm-commits]

On Mon, Aug 1, 2016 at 11:40 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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.
>
Yes, i pushed from the wrong tree, where i hadn't amended the commit
message  (which is actually very wrong).  I didn't want to rewrite the
commit message in the SVN repo, by changing the revprop, because i wasn't
sure whether it was wrong.

In the right tree, it actually says "Revert rev 276658, which fixed N^2
behavior in GVN-hoist, as it broke some of the build bots. See PR28670".


>
>
>>
>> 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/30272cb4/attachment.html>