<div dir="ltr">This change is definitely wrong.<div><br></div><div>Before, the ops and blocks were in the same order.</div><div>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.</div><div>Now, one array is sorted non-deterministically and one is not.</div><div><br></div><div>IE foo->ops[0] is not the op for block foo->block[0]</div><div><br></div><div>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.</div><div><br></div><div>So</div><div>1. They need to be in the same order</div><div>2. If you need a deterministic sort order, you would need to sort both.</div><div><br></div><div>You can see the constructor takes pains to sort the blocks, then push the ops in the same order</div><div>(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))</div><div><br></div><div><br></div><div>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.</div><div>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.</div><div>The walking cases are just copying or building these sets.</div><div><br></div><div>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.</div><div>(because whatever it is, sorting the ops and blocks differently isn't the right answer :P) </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 19, 2017 at 3:41 PM, Friedman, Eli via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_1416903213278165478moz-cite-prefix">Ping?<br>
<br>
-Eli<div><div class="h5"><br>
<br>
On 9/11/2017 10:01 AM, David Blaikie wrote:<br>
</div></div></div><div><div class="h5">
<blockquote type="cite">
<div dir="ltr">Good point Eli - Ben, thoughts?</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Aug 30, 2017 at 11:54 AM Friedman, Eli
via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <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>
>
==============================<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 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 Linux Foundation Collaborative Project<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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>
</blockquote>
</div>
</blockquote>
<p><br>
</p>
<pre class="m_1416903213278165478moz-signature" cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project</pre>
</div></div></div>
<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></blockquote></div><br></div>