[PATCH] D20116: Add speculatable function attribute

[Matt Arsenault via llvm-commits]

arsenm added a comment.

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.


The SpeculativeExecution pass already considers the cost

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.
>
> We should probably have another, symmetric, attribute: sinkable.  But that shouldn't be done with this patch.


We sort of already have this, convergent. Owen was talking about splitting convergent so that it really means do not sink, and then to add a speculatable attribute. The current convergent semantics would be the combination of no sink and no speculate

-Matt


https://reviews.llvm.org/D20116