<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 28, 2015 at 12:41 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">David,<br>
<br>
I think we're all on the same page. In the short term, however, this is a breaking change and should be reverted.<br></blockquote><div><br></div><div>Reverted in r246331.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks again,<br>
Hal<br>
<span class="im"><br>
----- Original Message -----<br>
> From: "Philip Reames" <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
> To: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>>, "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>
</span><div class=""><div class="h5">> Sent: Friday, August 28, 2015 12:03:38 PM<br>
> Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts are fair game for speculation<br>
><br>
><br>
> On 08/27/2015 07:40 PM, David Majnemer via llvm-commits wrote:<br>
><br>
> On Thu, Aug 27, 2015 at 5:37 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "David Majnemer" < <a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a> ><br>
> > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > Cc: "llvm-commits" < <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a> ><br>
><br>
><br>
> > Sent: Thursday, August 27, 2015 7:16:40 PM<br>
> > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts<br>
> > are fair game for speculation<br>
> ><br>
> ><br>
> > On Thu, Aug 27, 2015 at 5:09 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> > > From: "David Majnemer" < <a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a> ><br>
> > > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > > Cc: "llvm-commits" < <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a> ><br>
> > > Sent: Thursday, August 27, 2015 6:57:32 PM<br>
> > > Subject: Re: [llvm] r246232 - [ValueTracking] readnone CallInsts<br>
> > > are fair game for speculation<br>
> > ><br>
> > > On Thu, Aug 27, 2015 at 4:20 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > > wrote:<br>
> > ><br>
> > ><br>
> > > Hi David,<br>
> > ><br>
> > > I'd really like this to be a correct change, but I'm not sure<br>
> > > that<br>
> > > it<br>
> > > is. I'd like it to be correct because we add readnone for<br>
> > > functions<br>
> > > that, at the C level, have __attribute__((const)), and I believe<br>
> > > those can be speculated (assuming my understanding of GCC's<br>
> > > semantics is correct). However, in terms of readnone itself, we<br>
> > > need<br>
> > > to be careful about undefined behavior. Specifically,<br>
> > ><br>
> > > int divide(int x, int y) { return x/y; }<br>
> > ><br>
> > > is readnone, according to our definition, but could trap if<br>
> > > speculated with y == 0.<br>
> > ><br>
> > > Thanks again,<br>
> > > Hal<br>
> > ><br>
> > > Would it be correct for a function which non-deterministically<br>
> > > traps<br>
> > > (but which does not read or write memory) to be marked readnone?<br>
> ><br>
> > I believe this is correct, as the IR is currently defined. Plus<br>
> > FunctionAttrs will happily do this today (that divide function is<br>
> > marked readnone).<br>
> ><br>
> > > If<br>
> > > so, I think our optimizer is in quite a bit of trouble.<br>
> ><br>
> > :-)<br>
> ><br>
> > ><br>
> > > I'll also note that we go out of our way not to mark things like<br>
> > > rdrand/rdseed as readnone despite them not reading any memory.<br>
> > > I'd<br>
> > > prefer that readnone should imply that speculation, CSE, etc. are<br>
> > > safe. Would it be problematic to let 'divide' not be readnone?<br>
> > ><br>
> ><br>
> > I'd really prefer that we kept the speculation safety and the<br>
> > pointer<br>
> > aliasing properties separate, and so I don't want to mark divide as<br>
> > potentially accessing memory. I think that we should just add a<br>
> > safe_to_speculate attribute (modulo bikeshedding), and infer it by<br>
> > running isSafeToSpeculativelyExecute over the function in<br>
> > FunctionAttrs just like we do for readnone and mayWriteToMemory,<br>
> > etc.<br>
> ><br>
> ><br>
> ><br>
> > I'm not at all opposed to this resolution. However, I am bit<br>
> > confused. The langref states:<br>
> > "It does not write through any pointer arguments (including byval<br>
> > arguments) and never changes any state visible to callers. This<br>
> > means that it cannot unwind exceptions by calling the C++ exception<br>
> > throwing methods."<br>
> ><br>
> ><br>
> > It seems that readnone functions imply nounwind and "never changes<br>
> > any state visible to callers". Would trapping count?<br>
> ><br>
><br>
> I believe that it did not count prior to r246232 :-)<br>
><br>
> Seriously, however, I don't disagree with you. Trapping certainly<br>
> seems like it could be considered a behavior visible to the caller.<br>
> However, you could also argue that trapping terminates the program,<br>
> and the caller can't actually observe that. This latter viewpoint<br>
> seems to be the one guiding what we've currently implemented, and it<br>
> seems like a relatively-useful distinction.<br>
><br>
><br>
><br>
> I'm starting to split this gordian knot. One last question: the<br>
> 'nounwind' implication should be part of the new attribute, not the<br>
> old one right?<br>
> Sounds like you've reached the conclusion I would hope for. Thanks<br>
> for doing the work to introduce the speculatable attribute. That<br>
> will be really useful.<br>
><br>
> For the record, let me say that having readnone imply that a function<br>
> couldn't fault would be really problematic for me. A couple of<br>
> examples:<br>
> - readnone and readonly are documented as only describing memory<br>
> *visible to the program*. Functions so marked can still read and<br>
> write memory which is provably noalias with every pointer visible to<br>
> the program. This is really useful for caching and memoization<br>
> schemes inside runtime code where the implementation details aren't<br>
> visible to compiled code. Just because the memory isn't visible<br>
> (noalias) doesn't imply the dereferenceability of that memory isn't<br>
> controlled by actions taken by the compiled code.<br>
> - The division by zero case that already got mentioned. In<br>
> particular, there might have been a guarded divide within the<br>
> function whose guard was disproved by interprocedural analysis. Then<br>
> lifting the faulting divide above the control dependence in the<br>
> caller would be bad. We could require that all attributes be<br>
> stripped when removing the guard within the function, but that seems<br>
> less than helpful from an optimization perspective.<br>
><br>
> Philip<br>
><br>
<br>
</div></div><div class=""><div class="h5">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div></div>