simd_shuffle intrinsic: allow argument to be passed as vector #128731

RalfJung · 2024-08-06T10:30:07Z

See #128738 for context.

I'd like to get rid of this hack. #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.

rustbot · 2024-08-06T10:30:16Z

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-08-06T10:30:18Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

RalfJung · 2024-08-06T14:25:18Z

@antoyo the code here will need to be changed to work when mask is a struct or a vector. How could that be done?

antoyo · 2024-08-06T14:38:21Z

@antoyo the code here will need to be changed to work when mask is a struct or a vector. How could that be done?

If I understand correctly, you can skip all the code up to creating the mask vector if the argument is a vector already.
Unless you also need to adjust the size if different sizes are supported, where you will need to adjust the part pushing the missing elements.

RalfJung · 2024-08-06T14:45:21Z

The mask is required to be the same length as the output vector.

There's some code in there which seems to be concerned about the input vectors, though... those will often be shorter than the output vector, there's AFAIK no hard requirement here of any particular size (except that the two inputs have the same size).

RalfJung · 2024-08-06T14:53:59Z

There's also some odd casting going on... the mask has u32 elements but apparently that's not what GCC wants.

The complete lack of comments on most of the API makes it really painful to try and doing anything with this code. :/

RalfJung · 2024-08-06T14:58:03Z

I am looking for the array/vector equivalent of access_field, what does that look like? "Give me the n-th element of the vector (that I have as an RValue)"?

RalfJung · 2024-08-06T15:09:51Z

Okay I got something that builds. No idea if it makes any sense.^^ I am quite surprised that this seems to construct a while bunch of runtime computation for the shuffle index list; don't those have to be compile-time known?

antoyo · 2024-08-06T15:29:35Z

There's also some odd casting going on... the mask has u32 elements but apparently that's not what GCC wants.

The complete lack of comments on most of the API makes it really painful to try and doing anything with this code. :/

Yeah, the doc is only for the original C library for now.

Okay I got something that builds. No idea if it makes any sense.^^ I am quite surprised that this seems to construct a while bunch of runtime computation for the shuffle index list; don't those have to be compile-time known?

I don't remember for this specific case, but some operations like casts are allowed in const context.

Thanks for doing this and no worries: If it breaks when we sync back to the rustc_codegen_gcc repo, we'll fix it.

RalfJung · 2024-08-06T15:37:46Z

Thanks for doing this and no worries: If it breaks when we sync back to the rustc_codegen_gcc repo, we'll fix it.

Well, it will break in rustc CI when the stdarch changes I have planned as a follow-up to this get synced back to the rustc repo... so it might be a bit messy to sort out then. Currently this is dead code as the only user of simd_shuffle with a vector index is the ui test I changed, and that doesn't get run with GCC.

Ideally, the GCC backend wouldn't have its own test suite but would carry a list of files from the normal ui test suite that it should run -- that would avoid a ton of duplication of test cases...

BoxyUwU · 2024-08-08T00:22:40Z

r? @workingjubilee

…st as array)

RalfJung · 2024-08-13T05:53:49Z

#128537 landed, so this is now good to go. :)

workingjubilee · 2024-08-14T15:44:44Z

@workingjubilee today, yes?

RalfJung · 2024-08-24T14:41:21Z

@workingjubilee friendly reminder that this is still waiting for review. :)

workingjubilee

Oh, didn't realize half of it was gcc code I didn't need to review because antoyo already had a chance to look at it, heh. Looks fine to me.

workingjubilee · 2024-08-27T00:26:38Z

compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

+            // FIXME: this is a terrible abstraction-breaking hack.
+            // Find a way to reuse `immediate_const_vector` from `codegen_ssa` instead.


😅 I guess this is preexisting...

workingjubilee · 2024-08-27T00:30:42Z

compiler/rustc_codegen_llvm/src/intrinsic.rs

        // version of this intrinsic.
-        let n: u64 = match args[2].layout.ty.kind() {
+        let idx_ty = args[2].layout.ty;


"index type"?
isn't it more a "type at this index", so "this type's index"? Doesn't matter much, just curious.

It's the type of the argument named idx in the signature.

workingjubilee · 2024-08-27T00:33:51Z

tests/ui/simd/shuffle.rs

@@ -30,6 +35,17 @@ fn main() {
        let y: Simd<u8, 2> = simd_shuffle(a, b, I2);
        assert_eq!(y.0, [1, 5]);
    }
+    // Test that we can also use a SIMD vector instead of a normal array for the shuffle.
+    const I1_SIMD: Simd<u32, 4> = Simd([0, 2, 4, 6]);


Usually we avoid direct construction of Simd like this, but I suppose the fact that it occasionally emits illegal LLVMIR doesn't matter since this is const-evaluated. I suppose the other accesses and constructions here are preexisting, too, probably preceded that bug report, so not worth blocking this over since we should really try to scan the entire repo for them...

Is there a soundness bug where directly constructing SIMD values can miscompile? We shouldn't work around that in code, we should get codegen fixed...

Is this tracked anywhere?

@scottmcm Can you refresh my memory on what the issue was here?

Direct construction, as far as I know, is fine™. Both rust-lang/stdarch#1624 and (soon) #129403 have a bunch of it.

We might ban projecting into a simd type at some point in the future, because as I recall it's awkward for codegen, which doesn't want the lanes to be places, because it wants to extract/insert them instead of addressing them.

But that's not happened at this point, and is already in enough places that a few more is no big deal. And I think it might not even apply to Rvalue::Aggregate like seen here anyway, only to field projections.

(And, come to think of it, it might not even be a problem for array-based simd, because using the array as a place forces it to memory anyway. It makes me wonder if the problem was more about just .1 on a lots-of-fields simd type.)

We might ban projecting into a simd type at some point in the future, because as I recall it's awkward for codegen, which doesn't want the lanes to be places, because it wants to extract/insert them instead of addressing them.

Makes sense. However, if "awkward" leads to "generate wrong code" we should track that as an I-unsound issue somewhere.

I don't know any current "wrong code" cases, just recall that cg_clif had to do a bunch of extra work to make it work.

...turns out I should have looked harder: #137108 🙃

workingjubilee · 2024-08-27T00:34:07Z

tests/ui/simd/shuffle.rs

@@ -42,4 +58,11 @@ fn main() {
            Simd<u8, 16>,
        >(a, b);
    }
+    unsafe {
+        __shuffle_vector16_v2::<
+            { Simd([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]) },


Same thought as above.

workingjubilee · 2024-08-27T00:34:44Z

tests/ui/simd/shuffle.rs

 extern "rust-intrinsic" {
    fn simd_shuffle<T, I, U>(a: T, b: T, i: I) -> U;
 }


Hm, can't we just, like, import this from core::intrinsics now?

Yeah, this test is older than that.

workingjubilee · 2024-08-27T00:40:27Z

compiler/rustc_codegen_llvm/src/intrinsic.rs

+            _ if idx_ty.is_simd()
+                && matches!(
+                    idx_ty.simd_size_and_type(bx.cx.tcx).1.kind(),
+                    ty::Uint(ty::UintTy::U32)


I wonder if we actually need to enforce signedness? Though the 32-bitness is certainly fixed...

We enforce it in the array version so I figured we should keep enforcing it in the vector version.

workingjubilee · 2024-08-27T00:43:42Z

I'm feeling optimistic:
@bors r+ rollup

bors · 2024-08-27T00:43:44Z

📌 Commit daedbd4 has been approved by workingjubilee

It is now in the queue for this repository.

…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.

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension)) - rust-lang#127922 (Add unsafe to extern blocks in style guide) - rust-lang#128731 (simd_shuffle intrinsic: allow argument to be passed as vector) - rust-lang#128935 (More work on `zstd` compression) - rust-lang#128942 (miri weak memory emulation: put previous value into initial store buffer) - rust-lang#129418 (rustc: Simplify getting sysroot library directory) - rust-lang#129490 (Add Trusty OS as tier 3 target) - rust-lang#129559 (float types: document NaN bit pattern guarantees) - rust-lang#129642 (Bump backtrace to rust-lang/backtrace@fc37b22) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 9 pull requests Successful merges: - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension)) - rust-lang#127922 (Add unsafe to extern blocks in style guide) - rust-lang#128731 (simd_shuffle intrinsic: allow argument to be passed as vector) - rust-lang#128935 (More work on `zstd` compression) - rust-lang#128942 (miri weak memory emulation: put previous value into initial store buffer) - rust-lang#129418 (rustc: Simplify getting sysroot library directory) - rust-lang#129490 (Add Trusty OS as tier 3 target) - rust-lang#129536 (Add `f16` and `f128` inline ASM support for `aarch64`) - rust-lang#129559 (float types: document NaN bit pattern guarantees) r? `@ghost` `@rustbot` modify labels: rollup

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.

…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.

rustbot assigned BoxyUwU Aug 6, 2024

rustbot added S-waiting-on-review T-compiler T-libs labels Aug 6, 2024

This comment was marked as spam.

Sign in to view

RalfJung force-pushed the simd-shuffle-vector branch from 0962ef9 to 7cd4f58 Compare August 6, 2024 15:12

RalfJung mentioned this pull request Aug 6, 2024

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

Closed

This comment was marked as off-topic.

Sign in to view

RalfJung mentioned this pull request Aug 6, 2024

Deduplicate test suites for alternative codegen backends #128741

Open

This comment was marked as off-topic.

Sign in to view

rustbot assigned workingjubilee and unassigned BoxyUwU Aug 8, 2024

RalfJung added 2 commits August 13, 2024 07:51

simd_shuffle intrinsic: allow argument to be passed as vector (not ju…

194baa8

…st as array)

make the GCC backend compatible with vector shuffle indices

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

daedbd4

RalfJung force-pushed the simd-shuffle-vector branch from 7cd4f58 to daedbd4 Compare August 13, 2024 05:51

workingjubilee approved these changes Aug 27, 2024

View reviewed changes

workingjubilee closed this Aug 27, 2024

workingjubilee reopened this Aug 27, 2024

bors added S-waiting-on-bors and removed S-waiting-on-review labels Aug 27, 2024

workingjubilee mentioned this pull request Aug 27, 2024

Rollup of 9 pull requests #129643

Closed

tgross35 mentioned this pull request Aug 27, 2024

Rollup of 9 pull requests #129647

Merged

bors merged commit d2ff033 into rust-lang:master Aug 27, 2024
12 checks passed

rustbot added this to the 1.82.0 milestone Aug 27, 2024

RalfJung deleted the simd-shuffle-vector branch August 28, 2024 05:40

		// FIXME: this is a terrible abstraction-breaking hack.
		// Find a way to reuse `immediate_const_vector` from `codegen_ssa` instead.

simd_shuffle intrinsic: allow argument to be passed as vector #128731

simd_shuffle intrinsic: allow argument to be passed as vector #128731

Conversation

RalfJung commented Aug 6, 2024 • edited Loading

rustbot commented Aug 6, 2024

rustbot commented Aug 6, 2024

This comment was marked as spam.

RalfJung commented Aug 6, 2024

antoyo commented Aug 6, 2024

RalfJung commented Aug 6, 2024 • edited Loading

RalfJung commented Aug 6, 2024 • edited Loading

RalfJung commented Aug 6, 2024

RalfJung commented Aug 6, 2024

antoyo commented Aug 6, 2024

RalfJung commented Aug 6, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

BoxyUwU commented Aug 8, 2024

RalfJung commented Aug 13, 2024

workingjubilee commented Aug 14, 2024

RalfJung commented Aug 24, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Aug 27, 2024

bors commented Aug 27, 2024

RalfJung commented Aug 6, 2024 •

edited

Loading

RalfJung commented Aug 6, 2024 •

edited

Loading

RalfJung commented Aug 6, 2024 •

edited

Loading

RalfJung Aug 27, 2024 •

edited

Loading