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