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

[Hal Finkel via llvm-commits]

David,

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

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