<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 24, 2015 at 12:55 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 24 February 2015 at 02:05, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> Will try to reply to the larger thread again soon, but a quick reply on a<br>
> bit of a tangent...<br>
><br>
> On Mon, Feb 23, 2015 at 8:45 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
>><br>
>>  2. Should unreachable code be allowed to contain nonsense (like<br>
>> instructions that depend on themselves)? To this, I believe the answer is<br>
>> no. We currently permit this, and I think that a lot of the bugs regarding<br>
>> unreachable code some from this. I've yet to hear a good argument why, for<br>
>> example, JumpThreading must produce self-referential instructions in<br>
>> trivially-dead regions.<br>
><br>
><br>
> I could easily see tightening the rules on what passes the verifier within<br>
> unreachable code to try to make it substantially cheaper, easier, and less<br>
> error prone to handle unreachable code. It seems likely that there are good<br>
> pragmatic compromises here that would actually cost nothing. Obvious example<br>
> is, as you highlight, self-referential non-phi instructions could easily<br>
> just be rejected, and asserted during construction by the IR builder.<br>
<br>
</div></div>I am worried that the compromise might be worse than the two extremes<br>
in this case.<br></blockquote><div><br></div><div>Maybe.</div><div>My view is the ideal is either no-unreachable code, or unreachable blocks only contain terminators.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I don't think that rejecting<br>
<br>
  %a = getelementptr inbounds i8* %a, i64 1<br>
<br>
but accepting<br>
<br>
  %a = getelementptr inbounds i8* %b, i64 1<br>
  %b = getelementptr inbounds i8* %a, i64 1<br>
<br></blockquote><div><br></div><div>Does the verifier accept the latter right now?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">would get us much. It would probably just make unreachable handling<br>
bugs harder to find and test.<br>
<br>
I don't have a strong opinion so far, but it is interesting to note<br>
that this particular bug does showcase arguments on both sides:<br>
<br>
* The unreachable testcase that crashes gvn is simpler than the one<br>
that causes unreachable code to be produced.<br></blockquote><div><br></div><div>But this is because the passes cleanup unreachable code after themselves, whether they need to or not :)</div><div><br></div><div>FWIW, at least in every pass i've looked at, outside of jump threading, making them take unreachable code and "delete all instructions except the terminator, and make terminator trivial" would be completely and utterly trivial. In fact, almost *all* of them *already do this* (which is why this bug was hard to produce a testcase for, because you have to come up with a transformation that only one of the passes that *doesn't* clean up after itself will do something to).</div><div><br></div><div>This is separate than "what does the input to the compiler look like". I'm fine with unreachable code coming in (because such a thing is easy to cleanup/fix, and we in fact, already do this as the first real pass), i'm more concerned with the contract between passes.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
* Having the verifier reject unreachable code at the end of a pass<br>
would probably have made it easier to identify and reduce the issue,<br>
as the verifier would always detect the unreachable code but the code<br>
has to be in a particular shape to crash gvn.<br>
<br>
On the testcase reduction side of things, the only think I would<br>
insist on is that bb1 still counts as reachable in<br>
<br>
bb0:<br>
br i1 false, label %bb1, label %bb2<br></blockquote><div><br></div><div>Sure.<br></div><div>This is in fact, what i think the ideal for "leave unreachable blocks" around looks like - we don't mess with the cfg, but we do get rid of all other instructions/uses.</div><div>As i've said, this is what almost all passes *already do*.</div><div><br></div><div><br></div><div><br></div></div></div></div>