implement SIMD gather/scatter via vector getelementptr #95961

RalfJung · 2022-04-12T02:00:56Z

However, I don't really know what I am doing here... Cc @workingjubilee @calebzulawski

I didn't do anything for cranelift -- @bjorn3 not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

rust-highfive · 2022-04-12T02:00:58Z

Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary

rust-highfive · 2022-04-12T02:01:00Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

library/portable-simd/crates/core_simd/src/vector/ptr.rs

calebzulawski · 2022-04-12T03:20:10Z

compiler/rustc_codegen_llvm/src/intrinsic.rs

+        let layout = bx.layout_of(pointee.ty);
+        let ptrs = args[0].immediate();
+        let offsets = args[1].immediate();
+        return Ok(bx.gep(bx.backend_type(layout), ptrs, &[offsets]));


Does gep type check this? Or can calling the intrinsic improperly result in an LLVM codegen error?

I don't know, so we should probably add a codegen test rather than finding out the hard way. gep isn't that complex but it's known to ambush unwary compiler engineers when they turn their back on it. I can help draft such a codegen test if necessary.

This does about as much checking as the arith_offset intrinsic, I think. The one extra check there is that the offsets must be ptr-sized integers, which arith_offset enforces via the type signature (but I am not sure if anything checks that type signature). I can try to add that here.

What kind of codegen test are you looking for?

I added a check for the integer type in the second operand.

Oh, I mostly just want to see with this that we generate a vector of pointers to the given type and then gep it like we Damn Well Should and don't suddenly somehow revert to scalar operations or something like that. A smoke test that the suite of intrinsics used to do a gather or scatter compiles correctly, basically, and that the types don't go suddenly weird on us.

This does about as much checking as the arith_offset intrinsic, I think. The one extra check there is that the offsets must be ptr-sized integers, which arith_offset enforces via the type signature (but I am not sure if anything checks that type signature). I can try to add that here.

For the #[repr(simd)] types, while we do type-checking in rustc's front and "middle" phases to guarantee the input vector types are valid machine vector types, you can assume that it probably has bypassed any sensible checking like "is this even actually a pointer?" and is relying heavily on correct usage. Thus we would like to error during monomorphization on anything fishy.

That is to say:

It is important to error in mono (as you do here) because I do not believe we have anything before this step that would even enforce that the first arg to simd_arith_offset would be a vector of pointers.

Oh, I mostly just want to see with this that we generate a vector of pointers to the given type

Okay... I'll try my best but I barely ever work with codegen tests so I am not even sure of the syntax to use.^^ Is there another portable-simd codegen test I could model this off of?
Should it go through portable-simd APIs or call the intrinsics directly?

A smoke test that the suite of intrinsics used to do a gather or scatter compiles correctly, basically, and that the types don't go suddenly weird on us.

We have the doctests as smoke tests as well. ;)

RalfJung · 2022-04-13T02:26:24Z

All right, I added a codegen test. I don't know if that's what you were asking for, though.^^

bjorn3 · 2022-04-13T10:31:39Z

I didn't do anything for cranelift -- @bjorn3 not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

The functions using this new intrinsic are marked as #[inline] and thus only get codegened when actually using it, so this won't break anything when not using core::simd. I am fine with landing this as is. I will implement it in cg_clif at a later point.

workingjubilee · 2022-04-14T22:48:55Z

Oh yes, that is exactly what I had in mind!

The doctests are functional tests, which are good, but sometimes people noodle around with the ABI in a way that causes codegen-level regressions even though they don't impact functionality. They usually do it because it improves codegen in one place, but it's rarely worth it if it causes a regression on basic codegen elsewhere. These should rarely happen with what we are doing, but I have started becoming more concerned about paying close attention to what is happening with our emitted LLVMIR.

So as the test passes, and with bjorn3's assent, let's get those on the road.
This should be safe to roll up because it is purely additive.

@bors r+ rollup=always

bors · 2022-04-14T22:48:56Z

📌 Commit eb905c0 has been approved by workingjubilee

…ubilee implement SIMD gather/scatter via vector getelementptr Fixes rust-lang/portable-simd#271 However, I don't *really* know what I am doing here... Cc `@workingjubilee` `@calebzulawski` I didn't do anything for cranelift -- `@bjorn3` not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

…ubilee implement SIMD gather/scatter via vector getelementptr Fixes rust-lang/portable-simd#271 However, I don't *really* know what I am doing here... Cc ``@workingjubilee`` ``@calebzulawski`` I didn't do anything for cranelift -- ``@bjorn3`` not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

…ubilee implement SIMD gather/scatter via vector getelementptr Fixes rust-lang/portable-simd#271 However, I don't *really* know what I am doing here... Cc ```@workingjubilee``` ```@calebzulawski``` I didn't do anything for cranelift -- ```@bjorn3``` not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

Dylan-DPC · 2022-04-15T18:53:31Z

failed in rollup

@bors r-

RalfJung · 2022-04-15T19:02:39Z

D'oh, of course, that codegen test should probably be made 64bit-only.

RalfJung · 2022-04-15T19:04:20Z

@bors r=workingjubilee

bors · 2022-04-15T19:04:21Z

📌 Commit 73f9571 has been approved by workingjubilee

…ubilee implement SIMD gather/scatter via vector getelementptr Fixes rust-lang/portable-simd#271 However, I don't *really* know what I am doing here... Cc `@workingjubilee` `@calebzulawski` I didn't do anything for cranelift -- `@bjorn3` not sure if it's okay for that backend to temporarily break. I'm happy to cherry-pick a patch that adds cranelift support. :)

Rollup of 9 pull requests Successful merges: - rust-lang#93969 (Only add codegen backend to dep info if -Zbinary-dep-depinfo is used) - rust-lang#94605 (Add missing links in platform support docs) - rust-lang#95372 (make unaligned_references lint deny-by-default) - rust-lang#95859 (Improve diagnostics for unterminated nested block comment) - rust-lang#95961 (implement SIMD gather/scatter via vector getelementptr) - rust-lang#96004 (Consider lifetimes when comparing types for equality in MIR validator) - rust-lang#96050 (Remove some now-dead code that was only relevant before deaggregation.) - rust-lang#96070 ([test] Add test cases for untested functions for BTreeMap) - rust-lang#96099 (MaybeUninit array cleanup) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned Mark-Simulacrum Apr 12, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2022

calebzulawski reviewed Apr 12, 2022

View reviewed changes

library/portable-simd/crates/core_simd/src/vector/ptr.rs Show resolved Hide resolved

calebzulawski reviewed Apr 12, 2022

View reviewed changes

RalfJung added 2 commits April 12, 2022 11:09

add simd_arith_offset intrinsics

7f945b2

portable-simd: use simd_arith_offset to avoid ptr-int transmutation

e886dc5

RalfJung force-pushed the gather-scatter branch from 879e7c2 to e886dc5 Compare April 12, 2022 15:09

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 Apr 14, 2022

fee1-dead mentioned this pull request Apr 15, 2022

Rollup of 16 pull requests #96069

Closed

RalfJung mentioned this pull request Apr 15, 2022

Rollup of 8 pull requests #96075

Closed

RalfJung mentioned this pull request Apr 15, 2022

Rollup of 8 pull requests #96083

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 15, 2022

add codegen smoke test

73f9571

RalfJung force-pushed the gather-scatter branch from eb905c0 to 73f9571 Compare April 15, 2022 19:04

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 15, 2022

RalfJung mentioned this pull request Apr 15, 2022

Rollup of 10 pull requests #96093

Closed

Dylan-DPC mentioned this pull request Apr 16, 2022

Rollup of 9 pull requests #96108

Merged

bors merged commit ea131bc into rust-lang:master Apr 16, 2022

rustbot added this to the 1.62.0 milestone Apr 16, 2022

RalfJung deleted the gather-scatter branch April 16, 2022 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement SIMD gather/scatter via vector getelementptr #95961

implement SIMD gather/scatter via vector getelementptr #95961

RalfJung commented Apr 12, 2022

rust-highfive commented Apr 12, 2022

rust-highfive commented Apr 12, 2022

calebzulawski Apr 12, 2022

workingjubilee Apr 12, 2022 •

edited

Loading

RalfJung Apr 12, 2022 •

edited

Loading

RalfJung Apr 12, 2022

workingjubilee Apr 12, 2022

workingjubilee Apr 12, 2022 •

edited

Loading

RalfJung Apr 12, 2022 •

edited

Loading

RalfJung commented Apr 13, 2022

bjorn3 commented Apr 13, 2022

workingjubilee commented Apr 14, 2022 •

edited

Loading

bors commented Apr 14, 2022

Dylan-DPC commented Apr 15, 2022

RalfJung commented Apr 15, 2022

RalfJung commented Apr 15, 2022

bors commented Apr 15, 2022

implement SIMD gather/scatter via vector getelementptr #95961

implement SIMD gather/scatter via vector getelementptr #95961

Conversation

RalfJung commented Apr 12, 2022

rust-highfive commented Apr 12, 2022

rust-highfive commented Apr 12, 2022

calebzulawski Apr 12, 2022

Choose a reason for hiding this comment

workingjubilee Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Apr 12, 2022

Choose a reason for hiding this comment

workingjubilee Apr 12, 2022

Choose a reason for hiding this comment

workingjubilee Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Apr 13, 2022

bjorn3 commented Apr 13, 2022

workingjubilee commented Apr 14, 2022 • edited Loading

bors commented Apr 14, 2022

Dylan-DPC commented Apr 15, 2022

RalfJung commented Apr 15, 2022

RalfJung commented Apr 15, 2022

bors commented Apr 15, 2022

workingjubilee Apr 12, 2022 •

edited

Loading

RalfJung Apr 12, 2022 •

edited

Loading

workingjubilee Apr 12, 2022 •

edited

Loading

RalfJung Apr 12, 2022 •

edited

Loading

workingjubilee commented Apr 14, 2022 •

edited

Loading