[llvm] r276670 - Revert NewGVN N^2 behavior patch
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 1 12:41:02 PDT 2016
(and in fact, if we are okay with folks editing revprops, i'll happily go
and fix the commit message post-hoc)
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/3fda7627/attachment.html>
More information about the llvm-commits
mailing list