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

[Daniel Berlin via llvm-commits]

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/20170919/f22e97f7/attachment.html>