[PATCH] D20116: Add speculatable function attribute

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D20116#711882, @dberlin wrote:

> Right, my point was more on the side of "we are pretending we've got the semantics of these other instructions down really well and they are easy to understand", when, honestly, there is nothing in the langref that outlaws what i wrote :)
>
> The closest it comes is "when called with the same set of arguments and global state"
>  But you aren't calling it with the same global state  depending on the definition of global state, which .. we don't define anywhere.


Of course.  I'm not claiming that the LangRef is a mathematically precise document (though parts of it could be, if we incorporated Vellvm).  However, the converse of "it is mathematically precise" is not "anything goes". :)

In other words, I think despite the semantics of LLVM IR being only informally specified, we can still reasonably draw *some* boundaries on what the semantics of various constructs *can* be, based on the optimizations we want to be correct.

>> So even if there is some way to detect the difference, I'd consider that a bug in the LLVM IR semantics since it disallows an important optimization.
> 
> I guess my basic point was exactly this - the semantics of our other attributes, compared to speculatable, are not so easy or non-shocking that i think artificially limiting speculatable to non-callsites only makes sense.

I'm not sure if I agree with the "artificially" characterization.  My objection was based around (what I think are) concrete problems that we'll have if we allow this.

> Particularly becuase those attributes may not be so well defined that their behavior makes complete and total sense when applied to callsites

I agree we have room to improve today.  However, I don't see how two wrongs make a right here.

> Instead, when we discover they are wrong we fix them.  So why not just mark speculatable on callsites as experimental ,  see what happens, and go from there?
>  Bottom line: IMHO, We are unlikely to ever figure out the specific set of issues we will hit on callsites with speculatable if we don't allow it there.  Yeah, we'll figure out if it's completely broken if we don't, but we all seem to agree that it's probably not?

As I said, my objection was based on my opinion that they're already discovered to be wrong.  Of course, my (counter-)examples may not stand up to scrutiny, in which case my objection is moot.

Having said all this: while I won't exactly be happy with it, I would be fine with allowing `speculatable` on normal call sites if that helps some really compelling use case.  But I want to make it clear that we're making a tradeoff here.

> I'm going to drop the rest of this, fwiw, because i don't think it's worth pushing the readonly example any further.
>  I'm positive i can come up with a program that meets whatever requirements you throw at it and still breaks in your example, precisely because we don't define our IR well enough to prevent it (I'm on vacation, so i'm not going to think about whether i could prove that you can't make the IR well defined enough to prevent it and still have it express useful programs :P)


https://reviews.llvm.org/D20116