-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustc: Fix (again) simd vectors by-val in ABI #55073
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
3bd693d
to
c7a9b1b
Compare
cc @rkruppe |
src/rustllvm/DemoteSimd.cpp
Outdated
// out if we're interested in demoting this argument. | ||
bool anyVector = false; | ||
for (auto &Arg : F.args()) | ||
anyVector = anyVector || Arg.getType()->isVectorTy(); |
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.
Shouldn't this stop at the first argument that's a vector?:
for (auto &Arg : F.args()) {
if (Arg.getType()->isVectorTy()) {
Ret.push_back(&F);
break;
}
}
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.
A more concise way to write this test, which also short-circuits, is std::any_of(F.arg_begin(), F.arg_end(), [](Argument &Arg) { return ...; })
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.
Can one use llvm/ADT/STLExtras.h
here? This:
any_of(F.args(), [](auto &arg) { return arg.getType()->isVectorTy(); })
might work too.
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, sure, that works even better.
It might be worth it to run the |
@gnzlbg sure! I'm not sure there's an easy way to compare the output, but this is the output I got from nightly and this PR |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
c7a9b1b
to
fca80ac
Compare
@bors try |
rustc: Fix (again) simd vectors by-val in ABI The issue of passing around SIMD types as values between functions has seen [quite a lot] of [discussion], and although we thought [we fixed it][quite a lot] it [wasn't]! This PR is a change to rustc to, again, try to fix this issue. The fundamental problem here remains the same, if a SIMD vector argument is passed by-value in LLVM's function type, then if the caller and callee disagree on target features a miscompile happens. We solve this by never passing SIMD vectors by-value, but LLVM will still thwart us with its argument promotion pass to promote by-ref SIMD arguments to by-val SIMD arguments. This commit is an attempt to thwart LLVM thwarting us. We, just before codegen, will take yet another look at the LLVM module and demote any by-value SIMD arguments we see. This is a very manual attempt by us to ensure the codegen for a module keeps working, and it unfortunately is likely producing suboptimal code, even in release mode. The saving grace for this, in theory, is that if SIMD types are passed by-value across a boundary in release mode it's pretty unlikely to be performance sensitive (as it's already doing a load/store, and otherwise perf-sensitive bits should be inlined). The implementation here is basically a big wad of C++. It was largely copied from LLVM's own argument promotion pass, only doing the reverse. In local testing this... Closes #50154 Closes #52636 Closes #54583 Closes #55059 [quite a lot]: #47743 [discussion]: #44367 [wasn't]: #50154
@Mark-Simulacrum heh this pr does indeed make more sense for perf! I believe basically nothing in rustc uses explicit SIMD though, so I don't think this'll be a good source of benchmarks :( |
Hm, true, but I'm hopeful we can at least get manual or perhaps https://lolbench.rs/ results. |
@alexcrichton this does not appear to affect the performance of those benchmarks :) |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
fca80ac
to
2b4a369
Compare
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.
// argument promotion pass will promote these by-ref arguments to | ||
// by-val. That, however, introduces codegen errors! | ||
// | ||
// The upstream LLVM bug [1] has unfortunatey not really seen a lot of |
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.
Nit "unfortunatey" => "unfortunately"
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.
Ugh, is it nasty that it is necessary to do this at all.
I would strongly prefer an approach that either involves some sort of an attribute on arguments which block the promotion in the first place or a patch to LLVM that would stop LLVM from "promoting" vectors in arguments when the callee and caller disagree on features.
With the current behaviour and proposed PR we lose a lot of information pertaining to the by-pointer arguments, that was initially given by the compiler to LLVM, and expose ourselves to potentially very nasty to figure out ABI mismatch issues as well.
To elaborate on ABI issues, the most obvious example would be the changed codegen test which tests for (now mis-)matching C ABI. The less obvious one would be code generated by rustc itself, between its own codegen units… while it could very well be that it is not a problem at this particular moment, if anybody was to work in the related area in the future, I’m sure they’d be in for a lot of pain and confusion.
src/test/codegen/repr-transparent.rs
Outdated
@@ -108,7 +108,7 @@ struct f32x4(f32, f32, f32, f32); | |||
#[repr(transparent)] | |||
pub struct Vector(f32x4); | |||
|
|||
// CHECK: define <4 x float> @test_Vector(<4 x float> %arg0) | |||
// CHECK: define <4 x float> @test_Vector(<4 x float>* %arg0) | |||
#[no_mangle] | |||
pub extern fn test_Vector(_: Vector) -> Vector { loop {} } |
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.
Is this change really correct?
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.
It seems wrong to affect the C ABI, or any non-Rust ABI. We deliberately only pass vectors in memory for the Rust ABI, so in a sense this pass is currently doing far more than undoing argument promotion.
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 mentioned below, but passing SIMD values in FFI is unstable and not supported in rustc right now. It's correct that this demotion is blindly demoting all arguments, irrespective of ABI. That's not affecting stable users, however. I do not personally know of an easy fix for this (but would be willing to implement one!), and would prefer to not block the patch on a bug in what seems to be a rarely-used unstable feature
I agree! We have, however, waited months for an LLVM fix, responsibly filed an upstream bug, did all the bisection to get to the above state, and nothing is moving. It doesn't look like this is going to be fixed in LLVM unless we do it, and I do not know how to fix it in LLVM. I'm interested in getting a fix in rustc for this now, but if someone else were willing to send a patch to LLVM for this I'd be more than happy to delete all this. This PR is indeed not a great implementation but I believe it fits all our use cases on stable. We don't have a way to prevent promotion of these arguments so we're forcibly demoting them. In the code rustc generates the only promoted arguments are for internal functions which started out as demoted. FFI with SIMD is already unstable and has its own problems, so I have opted to not worry much here and if it's later decided to stabilize that it's a bug to fix then, I can certainly leave a comment to this effect though! I'd rather not be on the hook for blocking a bugfix affecting many users on stable while the implementation isn't perfect for possible future futures that aren't decided on yet. |
@nagisa I definitely agree with the sentiment (and suspect @alexcrichton does too) — but it was not clear to me if that was a "r=me (holding my nose)" sort of comment or if you had some concrete idea for an alternative approach. (Unless you meant to land a PR/patch to LLVM to prevent promotion in the first place.) |
As discussed on zulip, r=me if we skip external (or non-internal) functions for demotion. |
2b4a369
to
cfb4365
Compare
@bors: r=nagisa |
@bors r- A bunch of errors when building |
The issue of passing around SIMD types as values between functions has seen [quite a lot] of [discussion], and although we thought [we fixed it][quite a lot] it [wasn't]! This PR is a change to rustc to, again, try to fix this issue. The fundamental problem here remains the same, if a SIMD vector argument is passed by-value in LLVM's function type, then if the caller and callee disagree on target features a miscompile happens. We solve this by never passing SIMD vectors by-value, but LLVM will still thwart us with its argument promotion pass to promote by-ref SIMD arguments to by-val SIMD arguments. This commit is an attempt to thwart LLVM thwarting us. We, just before codegen, will take yet another look at the LLVM module and demote any by-value SIMD arguments we see. This is a very manual attempt by us to ensure the codegen for a module keeps working, and it unfortunately is likely producing suboptimal code, even in release mode. The saving grace for this, in theory, is that if SIMD types are passed by-value across a boundary in release mode it's pretty unlikely to be performance sensitive (as it's already doing a load/store, and otherwise perf-sensitive bits should be inlined). The implementation here is basically a big wad of C++. It was largely copied from LLVM's own argument promotion pass, only doing the reverse. In local testing this... Closes rust-lang#50154 Closes rust-lang#52636 Closes rust-lang#54583 Closes rust-lang#55059 [quite a lot]: rust-lang#47743 [discussion]: rust-lang#44367 [wasn't]: rust-lang#50154
12d79f7
to
3cc8f73
Compare
@bors: r=nagisa |
📌 Commit 3cc8f73 has been approved by |
⌛ Testing commit 3cc8f73 with merge 09eaa73cee26e37bd14248d0433be186c6f57a94... |
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors: retry |
The issue of passing around SIMD types as values between functions has seen [quite a lot] of [discussion], and although we thought [we fixed it][quite a lot] it [wasn't]! This PR is a change to rustc to, again, try to fix this issue. The fundamental problem here remains the same, if a SIMD vector argument is passed by-value in LLVM's function type, then if the caller and callee disagree on target features a miscompile happens. We solve this by never passing SIMD vectors by-value, but LLVM will still thwart us with its argument promotion pass to promote by-ref SIMD arguments to by-val SIMD arguments. This commit is an attempt to thwart LLVM thwarting us. We, just before codegen, will take yet another look at the LLVM module and demote any by-value SIMD arguments we see. This is a very manual attempt by us to ensure the codegen for a module keeps working, and it unfortunately is likely producing suboptimal code, even in release mode. The saving grace for this, in theory, is that if SIMD types are passed by-value across a boundary in release mode it's pretty unlikely to be performance sensitive (as it's already doing a load/store, and otherwise perf-sensitive bits should be inlined). The implementation here is basically a big wad of C++. It was largely copied from LLVM's own argument promotion pass, only doing the reverse. In local testing this... Closes rust-lang#50154 Closes rust-lang#52636 Closes rust-lang#54583 Closes rust-lang#55059 [quite a lot]: rust-lang#47743 [discussion]: rust-lang#44367 [wasn't]: rust-lang#50154
I proposed a fix for the LLVM bug here: https://reviews.llvm.org/D53554 For the work-around in rust, is there some reason why a new pass was written instead of just disabling llvm's ArgumentPromotion pass? |
Thanks so much @tstellar! I'll be very happy to remove this PR from our repo :) I didn't actually know that passes could be selectively disabled, but that probably would have been a better fix! |
It looks like this patch also causes segfaults in CI for stdsimd, so I've posted a revert. |
@alexcrichton You can't really disable most passes if you are using the default pass pipeline defined by one of LLVM's pass managers, but you can if you build your own pipeline manually. I wasn't sure if rust had its own pipeline or not. If not, it might be something to consider. |
@tstellar ah ok, makes sense! We're using the default pass managers currently in rustc so it sounds like we may not be able to turn it off easily. In any case waiting for your patch is probably the quickest way now :) |
The issue of passing around SIMD types as values between functions has
seen quite a lot of discussion, and although we thought we fixed
it it wasn't! This PR is a change to rustc to, again,
try to fix this issue.
The fundamental problem here remains the same, if a SIMD vector argument
is passed by-value in LLVM's function type, then if the caller and
callee disagree on target features a miscompile happens. We solve this
by never passing SIMD vectors by-value, but LLVM will still thwart us
with its argument promotion pass to promote by-ref SIMD arguments to
by-val SIMD arguments.
This commit is an attempt to thwart LLVM thwarting us. We, just before
codegen, will take yet another look at the LLVM module and demote any
by-value SIMD arguments we see. This is a very manual attempt by us to
ensure the codegen for a module keeps working, and it unfortunately is
likely producing suboptimal code, even in release mode. The saving grace
for this, in theory, is that if SIMD types are passed by-value across
a boundary in release mode it's pretty unlikely to be performance
sensitive (as it's already doing a load/store, and otherwise
perf-sensitive bits should be inlined).
The implementation here is basically a big wad of C++. It was largely
copied from LLVM's own argument promotion pass, only doing the reverse.
In local testing this...
Closes #50154
Closes #52636
Closes #54583
Closes #55059