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

[Hal Finkel]

----- Original Message -----
> From: "Rafael Espíndola" <rafael.espindola at gmail.com>
> To: "Chandler Carruth" <chandlerc at google.com>
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Katya Romanova" <Katya_Romanova at playstation.sony.com>, "LLVM Developers Mailing
> List" <llvmdev at cs.uiuc.edu>, "Rafael Espindola" <rafael_espindola at playstation.sony.com>
> Sent: Tuesday, February 24, 2015 2:55:16 PM
> Subject: Re: [LLVMdev] Jump Theading/GVN bug - moving discussion to llvm-dev
> 
> 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.

I completely agree with this. I'd like to see the same kind of 'instruction does not dominate all uses' errors as we get for reachable code for unreachable blocks in such cases.

 -Hal

> 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
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory