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

Fri Aug 28 14:21:02 PDT 2015

On Fri, Aug 28, 2015 at 12:41 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> David,
>
> I think we're all on the same page. In the short term, however, this is a
> breaking change and should be reverted.
>

Reverted in r246331.

> Thanks again,
> Hal
>
> ----- Original Message -----
> > From: "Philip Reames" <listmail at philipreames.com>
> > To: "David Majnemer" <david.majnemer at gmail.com>, "Hal Finkel" <
> hfinkel at anl.gov>
> > Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> > Sent: Friday, August 28, 2015 12:03:38 PM
> > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are
> fair game for speculation
> >
> >
> > 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 >
> > wrote:
> >
> >
> > ----- Original Message -----
> > > From: "David Majnemer" < david.majnemer at gmail.com >
> > > To: "Hal Finkel" < hfinkel at anl.gov >
> > > Cc: "llvm-commits" < 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 >
> > > wrote:
> > >
> > >
> > > ----- Original Message -----
> > > > From: "David Majnemer" < david.majnemer at gmail.com >
> > > > To: "Hal Finkel" < hfinkel at anl.gov >
> > > > Cc: "llvm-commits" < 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 >
> > > > 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
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150828/42a4d026/attachment.html>