[PATCH] D20116: Add speculatable function attribute

[David Majnemer via llvm-commits]

majnemer 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.


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.


================
Comment at: docs/LangRef.rst:1516-1518
@@ -1515,2 +1515,5 @@
     (dynamic thread safety analysis) are enabled for this function.
+``speculatable``
+    This function attribute indicates that the function does not have any
+    effects besides calculating the result and does not have undefined behavior.
 ``ssp``
----------------
We should say something to indicate that speculatable does not imply CSE-able.  Unless I am mistaken, it is possible for a function to be speculatable but return different results given the same parameters.


https://reviews.llvm.org/D20116