Skip to content
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

simd_shuffle cleanup: make the mask argument a vector (instead of an array) #128738

Closed
RalfJung opened this issue Aug 6, 2024 · 3 comments · Fixed by #130268
Closed

simd_shuffle cleanup: make the mask argument a vector (instead of an array) #128738

RalfJung opened this issue Aug 6, 2024 · 3 comments · Fixed by #130268
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2024

Since recently, codegen always passes constants of vector type as immediates in LLVM (#128537). This gives us a change to clean up this hack, where the idx argument of simd_shuffle is forced to be passed as an immediate. However, simd_shuffle for some reason actually takes an array as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This is more painful than it should be because stdarch is a submodule 😢 , so we'll have to do it in stages:

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 6, 2024
@RalfJung RalfJung added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Aug 6, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2024

FYI @oli-obk (making the hack a bit less hacky ;)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 6, 2024

yay.

Moving stdarch to josh first would help so many of these projects

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2024

Yeah, it would, but Amanieu had some concerns regarding CI.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Aug 27, 2024
…rkingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
tgross35 added a commit to tgross35/rust that referenced this issue Aug 27, 2024
…rkingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 27, 2024
Rollup merge of rust-lang#128731 - RalfJung:simd-shuffle-vector, r=workingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 28, 2024
simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang/rust#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang/rust#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Aug 28, 2024
simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang/rust#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang/rust#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
Zalathar added a commit to Zalathar/rust that referenced this issue Sep 14, 2024
…r=compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 14, 2024
…compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
@bors bors closed this as completed in a9dcd7f Sep 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 14, 2024
Rollup merge of rust-lang#130268 - RalfJung:simd-shuffle-idx-vector, r=compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
@saethlin saethlin added A-simd Area: SIMD (Single Instruction Multiple Data) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 15, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 16, 2024
…r-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang/rust#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang/rust#128738, see that issue for more context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants