Start letting through some more LLVM intrinsics disguised as calls #1565
Conversation
This is effectively an IR intrinsic, but is encoded as a call.
| @@ -1503,6 +1503,11 @@ impl<'a> Assemble<'a> { | |||
| "llvm.assume" => Ok(()), | |||
| "llvm.lifetime.start.p0" => Ok(()), | |||
| "llvm.lifetime.end.p0" => Ok(()), | |||
| x if x.starts_with("llvm.smax") => { | |||
There was a problem hiding this comment.
Do we know for sure that llvm.smax is never inlined at the MIR level?
There was a problem hiding this comment.
I see no evidence of that, but really it's beyond my purview.
There was a problem hiding this comment.
The problem is, if an intrinsic is inlined at the codegen level, then we trace the inlined version and then call it a second time.
That's probably OK (if a little iffy) for an operation like smax, but you certainly don't want to do that for operations that are expensive (in terms of performance, since you would incur the cost twice) or have side effects (which would break program semantics).
I'd like to hear what @ptersilie thinks, but my gut feeling is to not merge this until we have a better way of knowing whether an intrinsic has been inlined or not.
There was a problem hiding this comment.
Not merging this means that we are not compiling a surprising amount of traces: I have an extension of this PR that implements another one of these (ctpop) that triggers a bug (not, I'm about 95% sure, because of ctpop) that has been hidden because of all the traces we fail to compile.
|
We had an offline discussion and think this is safe. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
I currently cannot replicate this. Shall we retry? |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Closing this as so much has changed elsewhere that this PR needed redoing from scratch. |
Some of these are no-ops (c42bc99), some require codegen (e3f549d).