-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compatibility fixes for LLVM 3.5 #21588
Conversation
This commit introduce a third parameter for compatible_ifn!, as new intrinsics are being added in recent LLVM releases and there is no need to hardcode a specific case. Signed-off-by: Luca Bruno <lucab@debian.org>
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
compatible_ifn!("llvm.copysign.f64", copysign(t_f64, t_f64) -> t_f64, 4); | ||
compatible_ifn!("llvm.round.f32", roundf(t_f32) -> t_f32, 4); | ||
compatible_ifn!("llvm.round.f64", round(t_f64) -> t_f64, 4); | ||
compatible_ifn!("llvm.assume", fn(i1) -> void, 6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this isn't quite right, unfortunately; it seems like it might be declaring a substitute C function with signature void fn(i1)
. (I.e. the compatible_ifn!
macro has a different syntax.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ouch sorry. I didn't notice the difference in what the two macros expect as the second parameter.
260a406
to
f85e825
Compare
In turn, this seems to trigger an assertion in I was inclined to blame myself for something I've forgotten to touch, but it looks like the same ICE already surfaced before in #2235, so I am a bit unsure... |
Hm well the ICE may be related to some odd LLVM different, but we can probably diagnose after this lands, this is definitely good to land for now. Thanks! |
@bors: r+ f85e825 rollup |
Ok, I'll open a separate ticket for the ICE then, thanks. |
Signed-off-by: Luca Bruno <lucab@debian.org>
EngineBuilder signature has changed in LLVM 3.6, as the interface was not move-based in previous releases. Signed-off-by: Luca Bruno <lucab@debian.org>
f85e825
to
18f8d35
Compare
Mmmh, however I just realized that my fix was incomplete/incorrect as I was mapping "llvm.assume" to a non-existing "void assume(bool)" C function. Rebased a commit to introduce a stub function. However, in the ligth of #21620, I wonder if merging this PR makes sense anymore. |
Hm we'd probably prefer to lower it to a noop instead of a call to an actual function (as that may end up hindering optimizations). You've got a good point about #21620 though, so I think for now we can just close in favor of that. Thanks for investigating though! |
This fixes building with a system-installed LLVM 3.5 and also closes #20010