<div dir="ltr">FYI, I think you wrote  D36017 a few times when you meant  D36107, which caused it to link a completely unrelated review :)<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 31, 2017 at 1:41 PM, Tobias Grosser <span dir="ltr"><<a href="mailto:tobias.grosser@inf.ethz.ch" target="_blank">tobias.grosser@inf.ethz.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Jul 31, 2017, at 22:39, Tobias Grosser via llvm-commits wrote:<br>
> On Mon, Jul 31, 2017, at 22:29, Jakub Kuderski via Phabricator via<br>
> llvm-commits wrote:<br>
> > kuhar added a comment.<br>
> ><br>
> > In <a href="https://reviews.llvm.org/D36107#826573" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36107#826573</a>, @grosser wrote:<br>
> ><br>
> > > we have little tests that verify the update behavior of PDT after changes that introduce/remove virtual nodes. This is shown by the fact, that the first test case (construction of unreachables) illustrates a bug where we forget to add a new virtual exit node.<br>
> > ><br>
> > > I would like to commit this change, to have some more test coverage for <a href="https://reviews.llvm.org/D36107" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36107</a> and especially to make the impact of <a href="https://reviews.llvm.org/D36017" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36017</a> more clear.<br>
> ><br>
> ><br>
> > Just to clarify a bit: the current behavior posdominator updates is just<br>
> > broken when it comes to root handling -- it just happens that the<br>
> > verifier is broken in an analogous way and blindly trusts the set of<br>
> > roots inside a dominator tree. If for some reason we wanted keep not<br>
> > including infinite loops in postdominators, then it would be still<br>
> > possible to fix the root-finding behavior on updates. I mean, this issue<br>
> > is a bit orthogonal to what <a href="https://reviews.llvm.org/D36017" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36017</a> tries to do.<br>
><br>
> Sure. The goal of this patch was nothing else but to show the general<br>
> brokenness we see here. While looking through your patch closely and<br>
> trying to explore the current and future behavior some of this<br>
> brokenness popped up. I thought I better post these test cases as they<br>
> might help to fix future bugs and as they also will document well the<br>
> direction we are going.<br>
><br>
> I fully agree that the incorrect update behavior for unreachable<br>
> instructions is mostly independent of the goal of connecting infinite<br>
> loops to virtual exits.<br>
<br>
</span>Ah, the reason why <a href="https://reviews.llvm.org/D36017" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36017</a> is related is that it<br>
provides a possible solution to consistently handle unreachables and<br>
infinite loops.<br>
<br>
Best,<br>
Tobias<br>
</blockquote></div><br></div>