[PATCH] D20116: Add speculatable function attribute

[Hal Finkel via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D20116#484894, @majnemer wrote:

> In https://reviews.llvm.org/D20116#484877, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D20116#483719, @tstellarAMD wrote:
> >
> > > Rename the attribute to speculatable.  This simplifies the patch a lot since 
> > >  we are no longer trying to solve the problem  where specifying one of
> > >  the memory properties for intrinsics implies that it has no side effects
> > >  (this should probably still be addressed in another patch).
> > >
> > > I wasn't sure exactly how to define the speculatable attribute
> > >  in the language ref, so I copied the definition from the
> > >  isSafeToSpeculativelyExecute() function.
> >
> >
> > Makes sense to me. Is the idea to have isSafeToSpeculativelyExecute() return true on functions with this attribute? I'd find it strange if this were not the plan. Do we plan to have FuncAttrs infer the attribute? I'm wondering if we should say something about cost and how that will be handled. We don't want to speculate expensive-to-execute functions even if it is legal.
>
>
> I don't think we should avoid inferring speculatable via a cost model in the same way that we don't slap noinline on huge functions.
>  IMO, speculating function calls should likely be done via an IPO pass which is driven by some cost model.


I agree; although we'll need to think about how the API will work for this. We can't take our current code which says:

  if (isSafeToSpeculativelyExecute(I, ...)) {
    // speculate something
  }

and turn it into:

  if (!isa<CallInst>(I) && isSafeToSpeculativelyExecute(I, ...)) {
    // speculate something
  }

because there are intrinsics calls that isSafeToSpeculativelyExecute currently handles, and we'd like that to continue.

Maybe we need to add some 'PotentiallyExpensive' output to isSafeToSpeculativelyExecute. In any case, we can have this discussion in a separate review for the isSafeToSpeculativelyExecute change so long as it does not impact the design of the attribute itself.

> We should probably have another, symmetric, attribute: sinkable.  But that shouldn't be done with this patch.



https://reviews.llvm.org/D20116