[LLVMdev] Jump Theading/GVN bug - moving discussion to llvm-dev

[Rafael Espíndola]

On 24 February 2015 at 02:05, Chandler Carruth <chandlerc at google.com> wrote:
> Will try to reply to the larger thread again soon, but a quick reply on a
> bit of a tangent...
>
> On Mon, Feb 23, 2015 at 8:45 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>
>>  2. Should unreachable code be allowed to contain nonsense (like
>> instructions that depend on themselves)? To this, I believe the answer is
>> no. We currently permit this, and I think that a lot of the bugs regarding
>> unreachable code some from this. I've yet to hear a good argument why, for
>> example, JumpThreading must produce self-referential instructions in
>> trivially-dead regions.
>
>
> I could easily see tightening the rules on what passes the verifier within
> unreachable code to try to make it substantially cheaper, easier, and less
> error prone to handle unreachable code. It seems likely that there are good
> pragmatic compromises here that would actually cost nothing. Obvious example
> is, as you highlight, self-referential non-phi instructions could easily
> just be rejected, and asserted during construction by the IR builder.

I am worried that the compromise might be worse than the two extremes
in this case.

I don't think that rejecting

  %a = getelementptr inbounds i8* %a, i64 1

but accepting

  %a = getelementptr inbounds i8* %b, i64 1
  %b = getelementptr inbounds i8* %a, i64 1

would get us much. It would probably just make unreachable handling
bugs harder to find and test.

I don't have a strong opinion so far, but it is interesting to note
that this particular bug does showcase arguments on both sides:

* The unreachable testcase that crashes gvn is simpler than the one
that causes unreachable code to be produced.
* Having the verifier reject unreachable code at the end of a pass
would probably have made it easier to identify and reduce the issue,
as the verifier would always detect the unreachable code but the code
has to be in a particular shape to crash gvn.

On the testcase reduction side of things, the only think I would
insist on is that bb1 still counts as reachable in

bb0:
br i1 false, label %bb1, label %bb2

Cheers,
Rafael