[llvm] r312156 - [GVNSink] Remove dependency on SmallPtrSet iteration order.

Tue Sep 26 17:03:39 PDT 2017

+james

Yeah, i see there is a point where it does this:

  // The sunk instruction's results.
  ModelledPHI NewPHI(NewInsts, ActivePreds);

(Where activepreds is a smallptrset)
This can't give a right answer all the time.

I think the only way to construct it properly would be something like:

Create a vector from walking lri.blocks, and skipping items not in
activeblocks.
Pass that array to modelledphi.

If everything else is still in the same order, that should maintain it, i
believe.

On Mon, Sep 25, 2017 at 8:32 AM, Benjamin Kramer <benny.kra at gmail.com>
wrote:

> The reasoning here was that depending on the order of the SmallPtrSet
> (BArray is a SmallPtrSet) going into the ModelledPHI you get a result
> that's different from making one from the PHI directly. The iteration
> order of the pointers doesn't matter, but they have to be consistent
> to make operator== and hashing work properly.
>
> If I remember correctly I did the minimum change here to make tests
> pass, so the change was probably incomplete.
>
> On Wed, Sep 20, 2017 at 12:56 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
> > This change is definitely wrong.
> >
> > Before, the ops and blocks were in the same order.
> > This is the normal way phis are.  That way you can get the op and the
> block
> > for that op by the same index number.
> > Now, one array is sorted non-deterministically and one is not.
> >
> > IE foo->ops[0] is not the op for block foo->block[0]
> >
> > In fact, there is no way to recover the information you have lost here.
> You
> > cannot tell any longer which operand goes with which block.
> >
> > So
> > 1. They need to be in the same order
> > 2. If you need a deterministic sort order, you would need to sort both.
> >
> > You can see the constructor takes pains to sort the blocks, then push the
> > ops in the same order
> > (though it actually should do it the other way around.
> > getIncomingValueForBlock is O(N), while getIncomingBlock for each Use in
> the
> > operand list is O(1))
> >
> >
> > I've also stared at all the smallptrset usage in the file, and a glance,
> i
> > can't find a case where the output would be different due to iteration
> > order.
> > In the end, it looks like the only thing it really uses smallptrset for
> is
> > for testing whether things exist or don't exist in the set.
> > The walking cases are just copying or building these sets.
> >
> > I'm not sure what bug you think you've found here, but i believe you
> should
> > revert this, and explain in detail the problem you think this solves.
> > (because whatever it is, sorting the ops and blocks differently isn't the
> > right answer :P)
> >
> > On Tue, Sep 19, 2017 at 3:41 PM, Friedman, Eli via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Ping?
> >>
> >> -Eli
> >>
> >>
> >> On 9/11/2017 10:01 AM, David Blaikie wrote:
> >>
> >> Good point Eli - Ben, thoughts?
> >>
> >> On Wed, Aug 30, 2017 at 11:54 AM Friedman, Eli via llvm-commits
> >> <llvm-commits at lists.llvm.org> wrote:
> >>>
> >>> On 8/30/2017 11:46 AM, Benjamin Kramer via llvm-commits wrote:
> >>> > Author: d0k
> >>> > Date: Wed Aug 30 11:46:37 2017
> >>> > New Revision: 312156
> >>> >
> >>> > URL: http://llvm.org/viewvc/llvm-project?rev=312156&view=rev
> >>> > Log:
> >>> > [GVNSink] Remove dependency on SmallPtrSet iteration order.
> >>> >
> >>> > Found by LLVM_ENABLE_REVERSE_ITERATION.
> >>> >
> >>> > Modified:
> >>> >      llvm/trunk/lib/Transforms/Scalar/GVNSink.cpp
> >>> >
> >>> > Modified: llvm/trunk/lib/Transforms/Scalar/GVNSink.cpp
> >>> > URL:
> >>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Scalar/GVNSink.cpp?rev=312156&r1=312155&r2=312156&view=diff
> >>> >
> >>> > ============================================================
> ==================
> >>> > --- llvm/trunk/lib/Transforms/Scalar/GVNSink.cpp (original)
> >>> > +++ llvm/trunk/lib/Transforms/Scalar/GVNSink.cpp Wed Aug 30 11:46:37
> >>> > 2017
> >>> > @@ -229,12 +229,14 @@ public:
> >>> >     ModelledPHI(const VArray &V, const BArray &B) {
> >>> >       std::copy(V.begin(), V.end(), std::back_inserter(Values));
> >>> >       std::copy(B.begin(), B.end(), std::back_inserter(Blocks));
> >>> > +    std::sort(Blocks.begin(), Blocks.end());
> >>> >     }
> >>> >
> >>>
> >>> Does this actually solve anything?  "Blocks" is a
> >>> "SmallVector<BasicBlock *, 4>", so sorting it will put it into a
> >>> non-deterministic order.
> >>>
> >>> -Eli
> >>>
> >>> --
> >>> Employee of Qualcomm Innovation Center, Inc.
> >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> >>> Linux Foundation Collaborative Project
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at lists.llvm.org
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >>
> >> --
> >> Employee of Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux
> >> Foundation Collaborative Project
> >>
> >>
> >> _______________________________________________
> >> 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/20170926/04218dd7/attachment.html>