Add alignment parameter to `simd_masked_{load,store}` #147355

sayantn · 2025-10-04T23:41:43Z

This PR adds an alignment parameter in simd_masked_load and simd_masked_store, in the form of a const-generic enum core::intrinsics::simd::SimdAlign. This represents the alignment of the ptr argument in these intrinsics as follows

SimdAlign::Unaligned - ptr is unaligned/1-byte aligned
SimdAlign::Element - ptr is aligned to the element type of the SIMD vector (default behavior in the old signature)
SimdAlign::Vector - ptr is aligned to the SIMD vector type

The main motive for this is stdarch - most vector loads are either fully aligned (to the vector size) or unaligned (byte-aligned), so the previous signature doesn't cut it.

Now, stdarch will mostly use SimdAlign::Unaligned and SimdAlign::Vector, whereas portable-simd will use SimdAlign::Element.

cg_llvm
cg_clif
miri/const_eval

Alternatives

Using a const-generic/"const" u32 parameter as alignment (and we error during codegen if this argument is not a power of two). This, although more flexible than this, has a few drawbacks

If we use an const-generic argument, then portable-simd somehow needs to pass align_of::<T>() as the alignment, which isn't possible without GCE
"const" function parameters are just an ugly hack, and a pain to deal with in non-LLVM backends

We can remedy the problem with the const-generic u32 parameter by adding a special rule for the element alignment case (e.g. 0 can mean "use the alignment of the element type), but I feel like this is not as expressive as the enum approach, although I am open to suggestions

cc @workingjubilee @RalfJung @BoxyUwU

rustbot · 2025-10-04T23:41:46Z

The Miri subtree was changed

cc @rust-lang/miri

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

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_ssa

cc @WaffleLapkin

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

rustbot · 2025-10-04T23:41:48Z

r? @lcnr

rustbot has assigned @lcnr.
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

src/tools/miri/tests/pass/intrinsics/portable-simd.rs

compiler/rustc_codegen_llvm/src/intrinsic.rs

tests/codegen-llvm/simd-intrinsic/simd-intrinsic-generic-masked-store.rs

RalfJung · 2025-10-05T09:09:26Z

If we only need normally-aligned and unaligned loads, IMO it'd be better to just have a const generic boolean indicating which of them we want for any particular operation. That avoids ad-hoc hacks such as const parameters in intrinsics.

compiler/rustc_codegen_llvm/src/intrinsic.rs

programmerjake · 2025-10-05T09:56:25Z

If we only need normally-aligned and unaligned loads, IMO it'd be better to just have a const generic boolean indicating which of them we want for any particular operation. That avoids ad-hoc hacks such as const parameters in intrinsics.

for portable-simd I think we should default to element-level-alignment since I expect that to be more efficient than unaligned ops on some targets (GPUs? maybe RISC-V V?)

RalfJung · 2025-10-05T11:41:04Z

If we only need normally-aligned and unaligned loads, IMO it'd be better to just have a const generic boolean indicating which of them we want for any particular operation. That avoids ad-hoc hacks such as const parameters in intrinsics.

for portable-simd I think we should default to element-level-alignment since I expect that to be more efficient than unaligned ops on some targets (GPUs? maybe RISC-V V?)

Yes, so...?

IIUC we either want element-level alignment or no alignment, so we can just have a const bool generic controlling that.

programmerjake · 2025-10-05T12:21:22Z

If we only need normally-aligned and unaligned loads, IMO it'd be better to just have a const generic boolean indicating which of them we want for any particular operation. That avoids ad-hoc hacks such as const parameters in intrinsics.

for portable-simd I think we should default to element-level-alignment since I expect that to be more efficient than unaligned ops on some targets (GPUs? maybe RISC-V V?)

Yes, so...?

I thought you meant full-simd-type alignment or unaligned, since that's what x86 uses for simd instructions it calls aligned.

RalfJung · 2025-10-05T12:45:04Z

This is about simd_masked_load/store which are currently documented as

/// Unmasked values in `T` must be readable as if by `<ptr>::read` (e.g. aligned to the element /// type).

sayantn · 2025-10-05T12:47:49Z

As a summary, we need 3 types of alignments

element alignment (used in portable simd)
simd type alignment (x86 aligned)
fully unaligned (x86 unaligned)

So a bool flag won't cut it, at best we can use a const generic parameter, with 0 meaning element size aligned (because that is the most used, and can't be specified using const generics (requires gce))

RalfJung · 2025-10-05T12:53:19Z

As a summary, we need 3 types of alignments

Now you are expanding the scope of the PR. So far the motivation has been, we'd like an unaligned version of the existing intrinsics. If you also want SIMD type aligned variants, the PR description needs to be expanded to argue for this.

IIRC, last time this was looked into, the SIMD type alignment option wasn't necessary -- LLVM was more than able to use surrounding info on reference types to deduce the right alignment for the desired codegen. So please show some concrete undesirable codegen if you want to motivate a form of this intrinsic that requires SIMD type alignment.

sayantn · 2025-10-05T13:06:14Z

I apologise if I was unclear, but the motivation was always adding these 3 types of loads. LLVM will always generate an unaligned (byte-aligned) load/store if we pass any alignment less that the vector type size (because it is guaranteed to be safe). But for _mm_mask_load_ intrinsics, the pointer needs to be aligned to the vector size, so LLVM won't generate aligned loads unless we pass the vector size as alignment.

RalfJung · 2025-10-05T13:31:38Z

LLVM will always generate an unaligned (byte-aligned) load/store if we pass any alignment less that the vector type size

I don't think the "always" here is correct. If we are loading from an &__m128 LLVM should be able to use that fact to generate the aligned intrinsics.

However, I guess stdarch uses raw pointers in its API. So yeah this definitely needs to be explained properly in the PR description, currently it is at best confusing.

RalfJung · 2025-10-05T13:36:39Z

If we need 3 different alignment modes (Cc @Amanieu for the stdarch part here), that can still be done using const generics with a new 3-variant enum (similar to the enum we have for atomic memory access orderings).

workingjubilee · 2025-10-05T17:44:43Z

Yes, std::arch tries to reflect the type signatures used by the C vendor functions... it's not exactly just "bindgen for vendor functions", but it kinda is bindgen for vendor functions.

Amanieu · 2025-10-05T22:39:51Z

I'm happy with the current API that takes a constant (either as an argument or a const generic). An enum doesn't really provide much of an advantage when the desired alignment can just be explicitly provided.

sayantn · 2025-10-06T03:16:45Z

If everyone agrees, I can substitute the const argument for a const-generic u32 alignment. But then portable-simd would face problems because it has to pass align_of::<T> to the const-generic argument somehow, so I propose to add a special meaning to 0 - if 0 is passed as the alignment it is interpreted as the element type's alignment. This doesn't affect stdarch, all invocations of this intrinsic there will use literal (for x86 at least, I don't know much about other archs)

RalfJung · 2025-10-06T07:09:17Z

I'm happy with the current API that takes a constant (either as an argument or a const generic). An enum doesn't really provide much of an advantage when the desired alignment can just be explicitly provided.

The enum provides the big advantage that we don't need more ad-hock "constant argument" hacks.

What I was hoping to get from you is confirmation on which forms of the intrinsic are needed for stdarch.

RalfJung · 2025-10-06T07:10:43Z

If everyone agrees, I can substitute the const argument for a const-generic u32 alignment. But then portable-simd would face problems because it has to pass align_of::<T> to the const-generic argument somehow, so I propose to add a special meaning to 0 - if 0 is passed as the alignment it is interpreted as the element type's alignment. This doesn't affect stdarch, all invocations of this intrinsic there will use literal (for x86 at least, I don't know much about other archs)

I would prefer that over the "constant argument" hack. Not sure if it's better than a 3-value enum but 🤷 .

lcnr · 2025-10-06T13:35:11Z

r? @RalfJung though feel free to reassigned

programmerjake · 2025-10-09T20:45:47Z

src/tools/miri/tests/pass/intrinsics/portable-simd.rs

+ let vals = unsafe {
+ simd_masked_load::<_, _, _, { SimdAlign::Unaligned }>(
+ mask,
+ buf.as_ptr().byte_offset(1), // this is guaranteed to be unaligned


on AVR, alignment of u32 is 1, so the comment isn't always correct

I think it's fine to ignore tier 3 platforms here.

Also in those cases we can't get unaligned pointers at all, this works if align of u32 is not 1. Anyway this is just to check that Miri handles the alignment argument correctly, and doesn't panic

RalfJung

There are no Miri tests for SimdAlign::Vector, I think?

View changes since this review

RalfJung · 2025-10-10T07:10:28Z

@bjorn3 how should we handle cranelift here? Can you help with updating the intrinsics, or is it okay to temporarily regress?

bjorn3 · 2025-10-10T08:32:07Z

Basically for the case where individual lanes are unaligned, you did have to replace the MemFlags::trusted() argument of load and store with MemFlags::new().with_notrap(). MemFlags::trusted() is equivalent to MemFlags::new().with_notrap().with_aligned().

sayantn · 2025-10-10T09:50:10Z

@bjorn3 wow, that seems easy! Thanks.

And I guess we don't need to do anything fancy for the SimdAlign::Vector case, as that is basically just an optimization?

bjorn3 · 2025-10-10T11:55:29Z

Yeah, cg_clif currently loads and stores a lane at a time.

rustbot · 2025-10-10T15:19:30Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

sayantn · 2025-10-10T15:22:30Z

@RalfJung I have added tests for SimdAlign::Vector. Thanks for reminding me, the implementation actually had a bug - it was checking the pointer alignment with the alignment of the array obtained from projecting into the SIMD type.

@bjorn3 I tried to implement it in cg_clif, could you just check it once please?

@rustbot ready

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs

src/tools/miri/tests/fail/intrinsics/simd_masked_load_underaligned.rs

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs

sayantn · 2025-10-10T17:35:27Z

That's weird, the tests pass locally!

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

RalfJung · 2025-10-10T20:08:56Z

That's weird, the tests pass locally!

Maybe the --target i686-pc-windows-msvc part is crucial to reproduce the problem?

sayantn · 2025-10-10T20:32:07Z

Seems like it, what should we do? Seems like the test output is dependent on architecture.

rust-log-analyzer · 2025-10-10T22:11:26Z

The job x86_64-gnu-miri failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

tests/fail/weak_memory/weak_uninit.rs ... ok tests/fail/tree_borrows/reserved/int-protected-write.rs ... ok tests/fail/tree_borrows/reserved/cell-protected-write.rs ... ok FAILED TEST: tests/fail/intrinsics/simd_masked_load_underaligned.rs command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-YhdPFr" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/fail/intrinsics" "tests/fail/intrinsics/simd_masked_load_underaligned.rs" "--edition" "2021" "--target" "i686-pc-windows-msvc" error: test got exit status: 0, but expected 1 = note: compilation succeeded, but was expected to fail error: no output was emitted --- full stdout: FAILED TEST: tests/fail/intrinsics/simd_masked_store_underaligned.rs command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-YhdPFr" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/fail/intrinsics" "tests/fail/intrinsics/simd_masked_store_underaligned.rs" "--edition" "2021" "--target" "i686-pc-windows-msvc" error: test got exit status: 0, but expected 1 = note: compilation succeeded, but was expected to fail error: no output was emitted --- Location: /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.30.2/src/lib.rs:365 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1: <color_eyre[503d3e52f1227b6f]::config::EyreHook>::into_eyre_hook::{closure#0}<unknown> at <unknown source file>:<unknown line> 2: eyre[ee62eca60309ba8]::private::format_err<unknown> at <unknown source file>:<unknown line> 3: ui_test[6614206fa75e23c]::run_tests_generic::<ui_test[6614206fa75e23c]::default_file_filter, ui[dec2bd4cbaba463]::run_tests::{closure#1}, alloc[1dbecc4300bc0743]::boxed::Box<dyn ui_test[6614206fa75e23c]::status_emitter::StatusEmitter>><unknown> at <unknown source file>:<unknown line> 4: ui[dec2bd4cbaba463]::ui<unknown> at <unknown source file>:<unknown line> 5: ui[dec2bd4cbaba463]::main<unknown> at <unknown source file>:<unknown line> 6: std[5c593a824caf5f45]::sys::backtrace::__rust_begin_short_backtrace::<fn() -> core[2e4aa476c25a4c2]::result::Result<(), eyre[ee62eca60309ba8]::Report>, core[2e4aa476c25a4c2]::result::Result<(), eyre[ee62eca60309ba8]::Report>><unknown> at <unknown source file>:<unknown line> 7: std[5c593a824caf5f45]::rt::lang_start::<core[2e4aa476c25a4c2]::result::Result<(), eyre[ee62eca60309ba8]::Report>>::{closure#0}<unknown> at <unknown source file>:<unknown line> 8: std[5c593a824caf5f45]::rt::lang_start_internal<unknown> at <unknown source file>:<unknown line> 9: main<unknown> at <unknown source file>:<unknown line> 10: __libc_start_main<unknown> at <unknown source file>:<unknown line> 11: _start<unknown> at <unknown source file>:<unknown line> Run with COLORBT_SHOW_HIDDEN=1 environment variable to disable frame filtering. Run with RUST_BACKTRACE=full to include source snippets. error: test failed, to rerun pass `--test ui` Caused by: process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/deps/ui-eca9d5916b357dc3` (exit status: 1) Bootstrap failed while executing `test --stage 2 src/tools/miri src/tools/miri/cargo-miri --target i686-pc-windows-msvc` Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo test --target x86_64-unknown-linux-gnu -Zbinary-dep-depinfo -j 4 -Zroot-dir=/checkout --locked --color always --release --manifest-path /checkout/src/tools/miri/Cargo.toml -- [workdir=/checkout]` failed with exit code 1 Created at: src/bootstrap/src/core/build_steps/tool.rs:191:21 Executed at: src/bootstrap/src/core/build_steps/test.rs:677:19 Command has failed. Rerun with -v to see more details. Build completed unsuccessfully in 0:02:46 local time: Fri Oct 10 22:11:14 UTC 2025 network time: Fri, 10 Oct 2025 22:11:14 GMT ##[error]Process completed with exit code 1. Post job cleanup.

RalfJung · 2025-10-11T09:12:59Z

Well apparently your hypothesis that Simd::<i32, 5> has a larger alignment than i32 does not hold true on all targets. Maybe try Simd::<i32, 8>?
EDIT: Or maybe it's about the alignment of i32x4?

You can easily run tests on other targets yourself, Miri supports cross-interpretation. (You may have to shut up bootstrap's checks by setting BOOTSTRAP_SKIP_TARGET_SANITY=1).

RalfJung · 2025-10-11T09:19:11Z

Ah no, you were just lucky in your previous tests. Simd::<i32, 5> is 4-aligned so it can, by pure chance, be the case that adding 4 to that actually ends up 16-aligned. You'll need to make the test reliably produce an unaligned address, e.g. by starting with something 16-aligned and then adding 4 to that.

RalfJung · 2025-10-11T09:21:09Z

src/tools/miri/tests/fail/intrinsics/simd_masked_load_underaligned.rs

The names of the tests make little sense -- why is it "unaligned" and "underaligned"? This is about the two SimdAlign modes but those are not even mentioned.

Maybe ..._element_misaligned and ..._vector_misaligned?

rustbot assigned lcnr Oct 4, 2025

This comment has been minimized.

Sign in to view

sayantn force-pushed the masked-loads branch from 4cec11b to 9afbfb1 Compare October 5, 2025 00:27

programmerjake reviewed Oct 5, 2025

View reviewed changes

src/tools/miri/tests/pass/intrinsics/portable-simd.rs Outdated Show resolved Hide resolved

programmerjake reviewed Oct 5, 2025

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

programmerjake reviewed Oct 5, 2025

View reviewed changes

tests/codegen-llvm/simd-intrinsic/simd-intrinsic-generic-masked-store.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 5, 2025

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

rustbot assigned RalfJung Oct 6, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2025

programmerjake reviewed Oct 9, 2025

View reviewed changes

RalfJung requested changes Oct 10, 2025

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2025

sayantn force-pushed the masked-loads branch from 726481c to 108ca9d Compare October 10, 2025 15:19

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2025

RalfJung reviewed Oct 10, 2025

View reviewed changes

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 10, 2025

View reviewed changes

src/tools/miri/tests/fail/intrinsics/simd_masked_load_underaligned.rs Show resolved Hide resolved

RalfJung reviewed Oct 10, 2025

View reviewed changes

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/intrinsics/simd.rs Show resolved Hide resolved

Add implementation of the alignment parameter in Miri

857ba3e

sayantn force-pushed the masked-loads branch from 108ca9d to bea0ca8 Compare October 10, 2025 15:47

This comment has been minimized.

Sign in to view

bjorn3 reviewed Oct 10, 2025

View reviewed changes

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Outdated Show resolved Hide resolved

Implement the alignment parameter in cg_clif

2be31fc

sayantn force-pushed the masked-loads branch from bea0ca8 to 2be31fc Compare October 10, 2025 21:00

RalfJung reviewed Oct 11, 2025

View reviewed changes

Add alignment parameter to simd_masked_{load,store} #147355

Are you sure you want to change the base?

Add alignment parameter to simd_masked_{load,store} #147355

Conversation

sayantn commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Alternatives

rustbot commented Oct 4, 2025

rustbot commented Oct 4, 2025

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Oct 5, 2025

Uh oh!

programmerjake commented Oct 5, 2025

RalfJung commented Oct 5, 2025

programmerjake commented Oct 5, 2025

RalfJung commented Oct 5, 2025

sayantn commented Oct 5, 2025

RalfJung commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sayantn commented Oct 5, 2025

RalfJung commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

RalfJung commented Oct 5, 2025

workingjubilee commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Amanieu commented Oct 5, 2025

sayantn commented Oct 6, 2025

RalfJung commented Oct 6, 2025

RalfJung commented Oct 6, 2025

lcnr commented Oct 6, 2025

programmerjake Oct 9, 2025

Choose a reason for hiding this comment

RalfJung Oct 9, 2025

Choose a reason for hiding this comment

sayantn Oct 10, 2025

Choose a reason for hiding this comment

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

RalfJung commented Oct 10, 2025

bjorn3 commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sayantn commented Oct 10, 2025

bjorn3 commented Oct 10, 2025

rustbot commented Oct 10, 2025

sayantn commented Oct 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

sayantn commented Oct 10, 2025

Uh oh!

RalfJung commented Oct 10, 2025

sayantn commented Oct 10, 2025

rust-log-analyzer commented Oct 10, 2025

RalfJung commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

RalfJung commented Oct 11, 2025

RalfJung Oct 11, 2025

Choose a reason for hiding this comment

Labels

10 participants

Add alignment parameter to `simd_masked_{load,store}` #147355

Add alignment parameter to `simd_masked_{load,store}` #147355

sayantn commented Oct 4, 2025 •

edited

Loading

RalfJung commented Oct 5, 2025 •

edited

Loading

RalfJung commented Oct 5, 2025 •

edited

Loading

workingjubilee commented Oct 5, 2025 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading

bjorn3 commented Oct 10, 2025 •

edited

Loading

RalfJung commented Oct 11, 2025 •

edited

Loading