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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 1 12:54:36 PDT 2016


On Mon, Aug 1, 2016 at 12:41 PM Daniel Berlin <dberlin at dberlin.org> wrote:

> (and in fact, if we are okay with folks editing revprops, i'll happily go
> and fix the commit message post-hoc)
>

I don't think we usually bother doing that - not sure of the
ramifications/safety - I've always understood editing history was fraught
with peril/demons/etc, but don't really know.

Was just intended as an FYI for next time (& for the information to be
added to this thread at least - as you did, so people can find it here even
if it's not in the log itself) & as a reminder/encouragement for others
reading the list.


>
>
> On Mon, Aug 1, 2016 at 12:06 PM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>>
>> 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/97d05f5e/attachment-0001.html>


More information about the llvm-commits mailing list