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

[Benjamin Kramer via llvm-commits]

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
>>
>