Prototype using const generic for simd_shuffle IDX array #115933

oli-obk · 2023-09-18T14:45:21Z

TLDR: there is now a fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U; intrinsic that allows replacing

simd_shuffle(a, b, const { stuff })

with

simd_shuffle_generic::<_, _, {&stuff}>(a, b)

which makes the compiler implementations much simpler, if we manage to at some point eliminate simd_shuffle.

There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.

rustbot · 2023-09-18T14:45:33Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rustbot · 2023-09-18T15:10:47Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

RalfJung · 2023-09-18T18:04:35Z

Miri code LGTM. Would be nice to have a test case actually calling the intrinsic (in tests/pass/portable-simd.rs).

workingjubilee · 2023-09-22T03:04:26Z

Should this wait on that test case?

workingjubilee · 2023-09-22T02:34:22Z

tests/ui/simd/intrinsic/generic-elements.rs

+
+        const I2: &[u32] = &[0; 2];
+        simd_shuffle_generic::<i32, i32, I2>(0, 0);
+        //~^ ERROR expected SIMD input type, found non-SIMD `i32`


It should really say "scalar" in this case, but... probably not a real concern.

workingjubilee · 2023-09-22T03:09:14Z

tests/ui/simd/intrinsic/generic-elements.rs

+        simd_shuffle_generic::<_, i32x2, I8>(x, x);
+        //~^ ERROR expected return type of length 8, found `i32x2` with length 2


This seems wrong, it shouldn't error? Given:

lhs TxN vector

rhs TxN vector

i1xM mask vector

the shufflevector instruction produces a vector of type T but size M, and M > N is legitimate (especially in the case of "unifying two vectors where M == N*2").

I8 has length 8, the return type has length 2, so M here would be both 2 and 8 -- and hence it should error, no?

oh, I see. apologies, yes, that's correct. Hmm that's... annoying, but anything more fancy is probably too painful to implement at this level of the type system.

RalfJung · 2023-09-22T05:35:57Z

compiler/rustc_codegen_llvm/src/intrinsic.rs

+        let n = idx.len() as u64;
+
+        require_simd!(ret_ty, InvalidMonomorphization::SimdReturn { span, name, ty: ret_ty });
+        let (out_len, out_ty) = ret_ty.simd_size_and_type(bx.tcx());


(ret_len, ret_elem) would be more consistent with the in/out variables, I think?

it's copy paste from the simd_shuffle impl, but yes

There are 8 occurences now (there were 7) of this same code. We should probably deduplicate something here and reconsider the names, but not in this PR ^^

oli-obk · 2023-09-26T11:49:06Z

Turns out we didn't even have simd_shuffle tests in miri. Added both.

RalfJung · 2023-09-26T13:02:00Z

I'm pretty sure we had such tests in Miri, but probably only via portable-simd APIs. Cannot hurt to have some direct tests as well for special intrinsics like this one. :)

workingjubilee · 2023-09-29T08:09:47Z

Nice!

@bors r+

bors · 2023-09-29T08:09:49Z

📌 Commit a9030e6 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2023-09-30T04:05:30Z

⌛ Testing commit a9030e6 with merge 9136560...

bors · 2023-09-30T05:52:23Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 9136560 to master...

rust-timer · 2023-09-30T07:22:45Z

Finished benchmarking commit (9136560): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-3.0%, -0.6%]	2
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.8%]	2
All ❌✅ (primary)	-1.8%	[-3.0%, -0.6%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.369s -> 630.501s (-0.14%)
Artifact size: 317.41 MiB -> 317.39 MiB (-0.01%)

bjorn3 · 2023-10-02T13:06:10Z

compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

+            let total_len = lane_count * 2;
+
+            let indexes =
+                idx.iter().map(|idx| idx.unwrap_leaf().try_to_u16().unwrap()).collect::<Vec<u16>>();


This should have used u32. Will fix in https://github.com/bjorn3/rustc_codegen_cranelift

…gjubilee Prototype using const generic for simd_shuffle IDX array cc rust-lang#85229 r? `@workingjubilee` on the design TLDR: there is now a `fn simd_shuffle_generic<T, U, const IDX: &'static [u32]>(x: T, y: T) -> U;` intrinsic that allows replacing ```rust simd_shuffle(a, b, const { stuff }) ``` with ```rust simd_shuffle_generic::<_, _, {&stuff}>(a, b) ``` which makes the compiler implementations much simpler, if we manage to at some point eliminate `simd_shuffle`. There are some issues with this today though (can't do math without bubbling it up in the generic arguments). With this change, we can start porting the simple cases and get better data on the others.

Some codegen cleanups around SIMD checks See rust-lang#115933 (comment) for the reason. This PR essentially just deduplicates code by moving it into a macro

Rollup merge of rust-lang#116267 - oli-obk:simd_cleanups, r=petrochenkov Some codegen cleanups around SIMD checks See rust-lang#115933 (comment) for the reason. This PR essentially just deduplicates code by moving it into a macro

rustbot assigned workingjubilee Sep 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 18, 2023

Prototype using const generic for simd_shuffle IDX array

1a01e57

oli-obk force-pushed the simd_shuffle_const branch from 78fad84 to 1a01e57 Compare September 18, 2023 15:10

workingjubilee reviewed Sep 22, 2023

View reviewed changes

RalfJung reviewed Sep 22, 2023

View reviewed changes

Add tests for simd_shuffle{_generic}

a9030e6

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2023

oli-obk mentioned this pull request Sep 29, 2023

Some codegen cleanups around SIMD checks #116267

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2023

bors merged commit 9136560 into rust-lang:master Sep 30, 2023
12 checks passed

rustbot added this to the 1.74.0 milestone Sep 30, 2023

bjorn3 reviewed Oct 2, 2023

View reviewed changes

oli-obk deleted the simd_shuffle_const branch October 4, 2023 09:41

pvdrz mentioned this pull request Oct 4, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

oli-obk mentioned this pull request Feb 17, 2024

require simd_insert, simd_extract indices to be constants #121225

Merged

RalfJung mentioned this pull request Feb 17, 2024

simd_shuffle, simd_insert, simd_extract should use const generics #85229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype using const generic for simd_shuffle IDX array #115933

Prototype using const generic for simd_shuffle IDX array #115933

oli-obk commented Sep 18, 2023 •

edited

rustbot commented Sep 18, 2023

rustbot commented Sep 18, 2023

RalfJung commented Sep 18, 2023

workingjubilee commented Sep 22, 2023

workingjubilee Sep 22, 2023

workingjubilee Sep 22, 2023

RalfJung Sep 22, 2023

workingjubilee Sep 22, 2023

RalfJung Sep 22, 2023

oli-obk Sep 22, 2023

oli-obk Sep 26, 2023

oli-obk commented Sep 26, 2023

RalfJung commented Sep 26, 2023

workingjubilee commented Sep 29, 2023

bors commented Sep 29, 2023

bors commented Sep 30, 2023

bors commented Sep 30, 2023

rust-timer commented Sep 30, 2023

bjorn3 Oct 2, 2023

		simd_shuffle_generic::<_, i32x2, I8>(x, x);
		//~^ ERROR expected return type of length 8, found `i32x2` with length 2

Prototype using const generic for simd_shuffle IDX array #115933

Prototype using const generic for simd_shuffle IDX array #115933

Conversation

oli-obk commented Sep 18, 2023 • edited

rustbot commented Sep 18, 2023

rustbot commented Sep 18, 2023

RalfJung commented Sep 18, 2023

workingjubilee commented Sep 22, 2023

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

oli-obk commented Sep 26, 2023

RalfJung commented Sep 26, 2023

workingjubilee commented Sep 29, 2023

bors commented Sep 29, 2023

bors commented Sep 30, 2023

bors commented Sep 30, 2023

rust-timer commented Sep 30, 2023

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

oli-obk commented Sep 18, 2023 •

edited