- Notifications
You must be signed in to change notification settings - Fork13.9k
rustc: SIMD types use pointers in Rust's ABI#47743
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
(rust_highfive has picked a reviewer for you, use r? to override) |
Closed
src/librustc_trans/abi.rs Outdated
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.
Oh, intrinsics do end up using this code, I forgot about that.
src/librustc_trans/abi.rs Outdated
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.
Do we want this? If so we should also use it for other indirect arguments below. But I think that if you already have a memory location to pass in,byvalwill make an extra stack copy instead of reusing the original.
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.
Oh hm so this may or may not be correct, I sort of just assumed that it was naively!
I did find though that this is what clang does, it marks all SIMD arguments asbyval along with a pointer indirection behind them.
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'm not sure I understand, Clang often passes vectors as immediates? seehttps://godbolt.org/g/2cSZJD
Clang may be using byval in some circumstances where the ABI dictates it (I believe byval has some ABI implications that differ from just passing a pointer), but for the "Rust" ABI we have more leeway.
Member
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.
byval doesn't change correctness (as long as you're consistent), it's just a (platform) ABI choice, and we just don't do it for the Rust ABI. AFAIK it means "we have a pointer at the IR level but the data behind the pointer is passed on the stack" vs the default "this is a pointer like any other passed in a register (or the stack if we ran out of registers)" - but note that in the default case it's still just the pointer itself passed on the stack, not what it points to.
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.
@rkruppe oh interesting! I wonder if this is an integer/float difference? I've been usinghttps://godbolt.org/g/t4qzjA as testing ground which shows using the byval attribute...
Maybe it's 128/256 ABI? Unsure.
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.
@eddyb ok cool, I'll switch to justmake_indirect for now.
504c0f8 to9068e9cCompare@bors r+ |
📌 Commit 9068e9c has been approved by |
src/librustc_trans/abi.rs Outdated
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.
missingand ?
9068e9c toab55f08Compare@bors: r=eddyb |
📌 Commit ab55f08 has been approved by |
alexcrichton added a commit to alexcrichton/stdarch that referenced this pull requestJan 25, 2018
Inrust-lang/rust#47743 the SIMD types in the Rust ABI are being switched togetting passed through memory unconditionally rather than through LLVM'simmediate registers. This means that a bunch of our assert_instr invocationsstarted breaking as LLVM has more efficient methods of dealing with memory thanthe instructions themselves.This switches `assert_instr` to unconditionally use a shim that is an `extern`function which should revert back to the previous behavior, using the simd typesas immediates and testing the same.
alexcrichton added a commit to rust-lang/stdarch that referenced this pull requestJan 25, 2018
Inrust-lang/rust#47743 the SIMD types in the Rust ABI are being switched togetting passed through memory unconditionally rather than through LLVM'simmediate registers. This means that a bunch of our assert_instr invocationsstarted breaking as LLVM has more efficient methods of dealing with memory thanthe instructions themselves.This switches `assert_instr` to unconditionally use a shim that is an `extern`function which should revert back to the previous behavior, using the simd typesas immediates and testing the same.
@bors: r- gotta update a codegen test |
This commit changes the ABI of SIMD types in the "Rust" ABI to unconditionallybe passed via pointers instead of being passed as immediates. This should fix alongstanding issue,rust-lang#44367, where SIMD-using programs ended up showing very oddbehavior at runtime because the ABI between functions was mismatched.As a bit of a recap, this is sort of an LLVM bug and sort of an LLVM feature(today's behavior). LLVM will generate code for a function solely looking at thefunction it's generating, including calls to other functions. Let's then sayyou've got something that looks like:```llvmdefine void@foo() { ; no target features enabled call void@bar(<i64 x 4> zeroinitializer) ret void}define void@bar(<i64 x 4>) #0 { ; enables the AVX feature ...}```LLVM will codegen the call to `bar` *without* using AVX registers becauase `foo`doesn't have access to these registers. Instead it's generated with emulationthat uses two 128-bit registers. The `bar` function, on the other hand, willexpect its argument in an AVX register (as it has AVX enabled). This means we'vegot a codegen problem!Comments onrust-lang#44367 have some more contexutal information but the crux of theissue is that if we want SIMD to work in general we'll need to ensure thatwhenever a function calls another they ABI of the arguments being passed is inagreement.One possible solution to this would be to insert "shim functions" where whenevera `target_feature` mismatch is detected the compiler inserts a shim functionwhere you pass arguments via memory to the shim and then the shim loads thevalues and calls the target function (where the shim and the target have thesame target features enabled). This unfortunately is quite nontrivial toimplement in rustc today (especially when accounting for function pointers andsuch).This commit takes a different solution, *always* passing SIMD arguments throughmemory instead of passing as immediates. This strategy solves the problem at theLLVM layer because the ABI between two functions never uses SIMD registers. Thisalso shouldn't be a hit to performance because SIMD performance is thought tooften rely on inlining anyway, where a `call` instruction, even if using SIMDregisters, would be disastrous to performance regardless. LLVM should then bemore than capable of fixing all our memory usage to use registers instead afterenough inlining has been performed.Note that there's a few caveats to this commit though:* The "platform intrinsic" ABI is omitted from "always pass via memory". This ABI is used to define intrinsics like `simd_shuffle4` where LLVM and rustc need to have the arguments as an immediate.* Additionally this commit does *not* fix the `extern` ("C") ABI. This means that the bug inrust-lang#44367 can still happen when using non-Rust-ABI functions. My hope is that before stabilization we can ban and/or warn about SIMD types in these functions (as AFAIK there's not much motivation to belong there anyway), but I'll leave that for a later commit and if this is merged I'll file a follow-up issue.All in all this...Closesrust-lang#44367
ab55f08 to502de01Compare@bors: r=eddyb |
📌 Commit502de01 has been approved by |
3 tasks
alexcrichton added a commit to alexcrichton/rust that referenced this pull requestOct 14, 2018
The issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closesrust-lang#50154Closesrust-lang#52636Closesrust-lang#54583Closesrust-lang#55059[quite a lot]:rust-lang#47743[discussion]:rust-lang#44367[wasn't]:rust-lang#50154
bors added a commit that referenced this pull requestOct 14, 2018
rustc: Fix (again) simd vectors by-val in ABIThe issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closes#50154Closes#52636Closes#54583Closes#55059[quite a lot]:#47743[discussion]:#44367[wasn't]:#50154
alexcrichton added a commit to alexcrichton/rust that referenced this pull requestOct 16, 2018
The issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closesrust-lang#50154Closesrust-lang#52636Closesrust-lang#54583Closesrust-lang#55059[quite a lot]:rust-lang#47743[discussion]:rust-lang#44367[wasn't]:rust-lang#50154
kennytm added a commit to kennytm/rust that referenced this pull requestOct 18, 2018
rustc: Fix (again) simd vectors by-val in ABIThe issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closesrust-lang#50154Closesrust-lang#52636Closesrust-lang#54583Closesrust-lang#55059[quite a lot]:rust-lang#47743[discussion]:rust-lang#44367[wasn't]:rust-lang#50154
alexcrichton added a commit to alexcrichton/rust that referenced this pull requestOct 19, 2018
The issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closesrust-lang#50154Closesrust-lang#52636Closesrust-lang#54583Closesrust-lang#55059[quite a lot]:rust-lang#47743[discussion]:rust-lang#44367[wasn't]:rust-lang#50154
Manishearth added a commit to Manishearth/rust that referenced this pull requestOct 20, 2018
The issue of passing around SIMD types as values between functions hasseen [quite a lot] of [discussion], and although we thought [we fixedit][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 argumentis passed by-value in LLVM's function type, then if the caller andcallee disagree on target features a miscompile happens. We solve thisby never passing SIMD vectors by-value, but LLVM will still thwart uswith its argument promotion pass to promote by-ref SIMD arguments toby-val SIMD arguments.This commit is an attempt to thwart LLVM thwarting us. We, just beforecodegen, will take yet another look at the LLVM module and demote anyby-value SIMD arguments we see. This is a very manual attempt by us toensure the codegen for a module keeps working, and it unfortunately islikely producing suboptimal code, even in release mode. The saving gracefor this, in theory, is that if SIMD types are passed by-value acrossa boundary in release mode it's pretty unlikely to be performancesensitive (as it's already doing a load/store, and otherwiseperf-sensitive bits should be inlined).The implementation here is basically a big wad of C++. It was largelycopied from LLVM's own argument promotion pass, only doing the reverse.In local testing this...Closesrust-lang#50154Closesrust-lang#52636Closesrust-lang#54583Closesrust-lang#55059[quite a lot]:rust-lang#47743[discussion]:rust-lang#44367[wasn't]:rust-lang#50154
7 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
A-SIMDArea: SIMD (Single Instruction Multiple Data) S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. T-langRelevant to the language team
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit changes the ABI of SIMD types in the "Rust" ABI to unconditionally
be passed via pointers instead of being passed as immediates. This should fix a
longstanding issue,#44367, where SIMD-using programs ended up showing very odd
behavior at runtime because the ABI between functions was mismatched.
As a bit of a recap, this is sort of an LLVM bug and sort of an LLVM feature
(today's behavior). LLVM will generate code for a function solely looking at the
function it's generating, including calls to other functions. Let's then say
you've got something that looks like:
LLVM will codegen the call to
barwithout using AVX registers becauasefoodoesn't have access to these registers. Instead it's generated with emulation
that uses two 128-bit registers. The
barfunction, on the other hand, willexpect its argument in an AVX register (as it has AVX enabled). This means we've
got a codegen problem!
Comments on#44367 have some more contexutal information but the crux of the
issue is that if we want SIMD to work in general we'll need to ensure that
whenever a function calls another they ABI of the arguments being passed is in
agreement.
One possible solution to this would be to insert "shim functions" where whenever
a
target_featuremismatch is detected the compiler inserts a shim functionwhere you pass arguments via memory to the shim and then the shim loads the
values and calls the target function (where the shim and the target have the
same target features enabled). This unfortunately is quite nontrivial to
implement in rustc today (especially when accounting for function pointers and
such).
This commit takes a different solution,always passing SIMD arguments through
memory instead of passing as immediates. This strategy solves the problem at the
LLVM layer because the ABI between two functions never uses SIMD registers. This
also shouldn't be a hit to performance because SIMD performance is thought to
often rely on inlining anyway, where a
callinstruction, even if using SIMDregisters, would be disastrous to performance regardless. LLVM should then be
more than capable of fixing all our memory usage to use registers instead after
enough inlining has been performed.
Note that there's a few caveats to this commit though:
The "platform intrinsic" ABI is omitted from "always pass via memory". This
ABI is used to define intrinsics like
simd_shuffle4where LLVM and rustcneed to have the arguments as an immediate.
Additionally this commit doesnot fix the
extern("C") ABI. This meansthat the bug inrepr(simd) is unsound #44367 can still happen when using non-Rust-ABI functions. My
hope is that before stabilization we can ban and/or warn about SIMD types in
these functions (as AFAIK there's not much motivation to belong there anyway),
but I'll leave that for a later commit and if this is merged I'll file a
follow-up issue.
All in all this...
Closes#44367