[PATCH] D20116: Add speculatable function attribute

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D20116#707126, @sanjoy wrote:

> In https://reviews.llvm.org/D20116#707100, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D20116#707083, @sanjoy wrote:
> >
> > > `readnone` etc. are different from `speculatable`, in that once you mark a call site as `speculatable` you've the said call site as speculatable throughout the lifetime of the program (since, by definition, it can be arbitrarily speculated).  `readnone`, `readonly` etc. do not have that property.
> >
> >
> > I don't follow why `readnone` and `readonly` based transformations don't need the same guarantee?
>
>
> I'm talking about cases like this:
>
>   void f(bool to_write, int* ptr) {
>     if (to_write) *ptr = 20;
>   }
>  
>   void g() {
>     int a;
>     f(false, &a) readnone;
>   }
>
>
> Now `f` is not readnone generally (there are argument values for which it writes to memory), but in that specific call site we know that it does not.
>
> What I was trying to say before is that, because of how we're defined `speculatable`, "`speculatable` for this specific argument / control dependence" does not make sense -- if you expand it out, the statement would be "for this specific set of arguments, global memory state and control dependence, this function does not have undefined behavior for any possible combination of arguments or global memory state", which contradicts itself.
>
> Of course, due to this attribute, we will start being able to hoist calls and invokes.  If those are marked `readnone` etc. we will have to strip those attributes if we hoisted said call or invoke out of control flow.  But that's a different story.


But I think this is exactly the point. We can consider value constraints due to data dependencies, but not from control dependencies. The same infects the other attributes as well (if it is tagged readnone and speculatable, the readnone can also not be control dependent - attributes are not metadata; we shouldn't be stripping them).


https://reviews.llvm.org/D20116