[PATCH] D20116: Add speculatable function attribute

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D20116#741130, @arsenm wrote:

> In https://reviews.llvm.org/D20116#741087, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D20116#740922, @arsenm wrote:
> >
> > > Only allow for intrinsics
> >
> >
> > Why only for intrinsics? I thought we had concluded that we'd only allow it for declarations and not on call sites (which may technically mean on call sited but only matching the declaration). I think it is important that we can apply it to regular functions.
>
>
> I think so too, but @sanjoy said to restrict it to intrinsics for now.


He suggested that, and I said that I did not want that restriction, and he said that he was fine with that:

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

> In https://reviews.llvm.org/D20116#708332, @hfinkel wrote:
>
> > I'm fine with restricting speculatable to only appear where it appears on a function declaration/definition unless/until we can figure out semantics for it on a call site in general. I don't want it restricted to intrinsics specifically, but I don't think that's the problem.
>
>
> Only function-level `speculatable` (and no call site specific `speculatable`) seems less problematic.  It would mean having a function declaration or definition incorrectly marked as `speculatable`, even if it is never called, is UB; but I can live with that as long as that is properly documented.




> Intrinsics are the important part.

Not to me ;)

> I also ran into one minor issue with the call site restriction in https://reviews.llvm.org/D32655 where speculatable intrinsic calls are sometimes replaced with non-speculatable libcalls.

This seems like it is an unfortunate information loss that we should fix, but why is that a problem?


https://reviews.llvm.org/D20116