[llvm] r246232 - [ValueTracking] readnone CallInsts are fair game for speculation

[Philip Reames via llvm-commits]

On 08/27/2015 07:40 PM, David Majnemer via llvm-commits wrote:
>
>
> On Thu, Aug 27, 2015 at 5:37 PM, Hal Finkel <hfinkel at anl.gov 
> <mailto:hfinkel at anl.gov>> wrote:
>
>     ----- Original Message -----
>     > From: "David Majnemer" <david.majnemer at gmail.com
>     <mailto:david.majnemer at gmail.com>>
>     > To: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>
>     > Cc: "llvm-commits" <llvm-commits at lists.llvm.org
>     <mailto:llvm-commits at lists.llvm.org>>
>     > Sent: Thursday, August 27, 2015 7:16:40 PM
>     > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts
>     are fair game for speculation
>     >
>     >
>     > On Thu, Aug 27, 2015 at 5:09 PM, Hal Finkel < hfinkel at anl.gov
>     <mailto:hfinkel at anl.gov> >
>     > wrote:
>     >
>     >
>     > ----- Original Message -----
>     > > From: "David Majnemer" < david.majnemer at gmail.com
>     <mailto:david.majnemer at gmail.com> >
>     > > To: "Hal Finkel" < hfinkel at anl.gov <mailto:hfinkel at anl.gov> >
>     > > Cc: "llvm-commits" < llvm-commits at lists.llvm.org
>     <mailto:llvm-commits at lists.llvm.org> >
>     > > Sent: Thursday, August 27, 2015 6:57:32 PM
>     > > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts
>     > > are fair game for speculation
>     > >
>     > > On Thu, Aug 27, 2015 at 4:20 PM, Hal Finkel < hfinkel at anl.gov
>     <mailto:hfinkel at anl.gov> >
>     > > wrote:
>     > >
>     > >
>     > > Hi David,
>     > >
>     > > I'd really like this to be a correct change, but I'm not sure that
>     > > it
>     > > is. I'd like it to be correct because we add readnone for
>     functions
>     > > that, at the C level, have __attribute__((const)), and I believe
>     > > those can be speculated (assuming my understanding of GCC's
>     > > semantics is correct). However, in terms of readnone itself, we
>     > > need
>     > > to be careful about undefined behavior. Specifically,
>     > >
>     > > int divide(int x, int y) { return x/y; }
>     > >
>     > > is readnone, according to our definition, but could trap if
>     > > speculated with y == 0.
>     > >
>     > > Thanks again,
>     > > Hal
>     > >
>     > > Would it be correct for a function which non-deterministically
>     > > traps
>     > > (but which does not read or write memory) to be marked readnone?
>     >
>     > I believe this is correct, as the IR is currently defined. Plus
>     > FunctionAttrs will happily do this today (that divide function is
>     > marked readnone).
>     >
>     > > If
>     > > so, I think our optimizer is in quite a bit of trouble.
>     >
>     > :-)
>     >
>     > >
>     > > I'll also note that we go out of our way not to mark things like
>     > > rdrand/rdseed as readnone despite them not reading any memory. I'd
>     > > prefer that readnone should imply that speculation, CSE, etc. are
>     > > safe. Would it be problematic to let 'divide' not be readnone?
>     > >
>     >
>     > I'd really prefer that we kept the speculation safety and the
>     pointer
>     > aliasing properties separate, and so I don't want to mark divide as
>     > potentially accessing memory. I think that we should just add a
>     > safe_to_speculate attribute (modulo bikeshedding), and infer it by
>     > running isSafeToSpeculativelyExecute over the function in
>     > FunctionAttrs just like we do for readnone and mayWriteToMemory,
>     > etc.
>     >
>     >
>     >
>     > I'm not at all opposed to this resolution. However, I am bit
>     > confused. The langref states:
>     > "It does not write through any pointer arguments (including byval
>     > arguments) and never changes any state visible to callers. This
>     > means that it cannot unwind exceptions by calling the C++ exception
>     > throwing methods."
>     >
>     >
>     > It seems that readnone functions imply nounwind and "never changes
>     > any state visible to callers". Would trapping count?
>     >
>
>     I believe that it did not count prior to r246232 :-)
>
>     Seriously, however, I don't disagree with you. Trapping certainly
>     seems like it could be considered a behavior visible to the
>     caller. However, you could also argue that trapping terminates the
>     program, and the caller can't actually observe that. This latter
>     viewpoint seems to be the one guiding what we've currently
>     implemented, and it seems like a relatively-useful distinction.
>
>
> I'm starting to split this gordian knot.  One last question: the 
> 'nounwind' implication should be part of the new attribute, not the 
> old one right?
Sounds like you've reached the conclusion I would hope for.  Thanks for 
doing the work to introduce the speculatable attribute.  That will be 
really useful.

For the record, let me say that having readnone imply that a function 
couldn't fault would be really problematic for me.  A couple of examples:
- readnone and readonly are documented as only describing memory 
*visible to the program*.  Functions so marked can still read and write 
memory which is provably noalias with every pointer visible to the 
program.  This is really useful for caching and memoization schemes 
inside runtime code where the implementation details aren't visible to 
compiled code.  Just because the memory isn't visible (noalias) doesn't 
imply the dereferenceability of that memory isn't controlled by actions 
taken by the compiled code.
- The division by zero case that already got mentioned.  In particular, 
there might have been a guarded divide within the function whose guard 
was disproved by interprocedural analysis. Then lifting the faulting 
divide above the control dependence in the caller would be bad.  We 
could require that all attributes be stripped when removing the guard 
within the function, but that seems less than helpful from an 
optimization perspective.

Philip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150828/b8d79a14/attachment.html>