<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 15, 2017 at 12:20 PM, Davide Italiano via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davide added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D34135#781243" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34135#781243</a>, @anna wrote:<br>
<br>
> In <a href="https://reviews.llvm.org/D34135#780695" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34135#780695</a>, @dberlin wrote:<br>
><br>
> > In <a href="https://reviews.llvm.org/D34135#780666" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34135#780666</a>, @anna wrote:<br>
> ><br>
> > > In <a href="https://reviews.llvm.org/D34135#780637" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34135#780637</a>, @efriedma wrote:<br>
> > ><br>
> > > > The sequence here is that we constant-fold a terminator (deleting an edge), then continue iterating through all the blocks in the function, and eventually crash when LVI discovers a cycle in a loop which was made unreachable.<br>
> > ><br>
> > ><br>
> > > So, the main problem here is that in each iteration of Jump threading, we can find the trivially dead blocks, but not the complete set of unreachable blocks. Also, I'm assuming transitively detecting dead blocks after the call to `processBlock` does not help in this case?<br>
> ><br>
> ><br>
> > The set of blocks it will find will depend on input order of basic blocks currently.<br>
> >  It is possible to increase it to find all non-cycle involved unreachable blocks  by performing the processing in the optimal iteration order.<br>
> >  It should be possible to find all unreachable blocks (including those involved in cycle) as you go by forming SCC's and iterating them individually in the right order.<br>
><br>
><br>
> Agreed. Also, there was even a discussion on changing the ordering for JumpThreading because of how the current ordering can cause certain blocks to be dead, and AA chokes on the PHIs in those blocks: <a href="https://bugs.llvm.org/show_bug.cgi?id=33142#c14" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_<wbr>bug.cgi?id=33142#c14</a><br>
>  (This might be completely irrelevant for this patch, but it's around the same problem of identifying  unreachability at all times in JumpThreading).<br>
<br>
<br>
</span>Yes. As the person who did the analysis on that bug I decided to not go that direction (i.e. use a stack worklist) because I wasn't able to prove myself it was correct.<br>
I've seen at least another example internally where JumpThreading goes bananas because of bad iteration ordering. Iterating SCC's topologically will solve at least part of  these problems,<br>
I think, but I'm afraid that's a much larger change. </blockquote><div><br></div><div>Yup</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I wouldn't stop this from going in but at the same time it's likely Zhendong's fuzzer (or other fuzzers, or simply production code :) will uncover similar issues to this.<br></blockquote><div><br></div><div>+1 to that.</div><div><br></div><div>I have no problem patching this in as long as we know it's just the start of a long ride.</div><div>In some ways, unless *this* bug is important, and more important than the other 15 bugs it's gonna find that we *aren't* going to be able to fix without rewriting it, i kinda wouldn't bother.</div><div><br></div><div>If they are important (IE occurring on real world code that matters, etc), well, good luck :)</div><div><br></div></div></div></div>