add core::intrinsics::simd::{simd_extract_dyn, simd_insert_dyn}

[folkertdev]

fixes #137372

adds core::intrinsics::simd::{simd_extract_dyn, simd_insert_dyn}, which contrary to their non-dyn counterparts allow a non-const index. Many platforms (but notably not x86_64 or aarch64) have dedicated instructions for this operation, which stdarch can emit with this change.

Future work is to also make the Index operation on the Simd type emit this operation, but the intrinsic can't be used directly. We'll need some MIR shenanigans for that.

r? @ghost

[folkertdev]

right so this works with the LLVM implementation (so the fallback is not used), is there some try job that would run tests/ui/simd/intrinsic/generic-elements-pass.rs with the fallback (e.g. with miri?)

[workingjubilee]

you should be able to just ./x.py test miri I think

[folkertdev]

I don't think that runs this test? or it does work. I've added a commit that disables the custom LLVM implementation, so the fallback should be used. Locally that errors with

--- stderr -------------------------------
error: internal compiler error: /home/folkertdev/rust/rust/compiler/rustc_codegen_ssa/src/mir/block.rs:987:29: intrinsic simd_insert_dyn must be overridden by codegen backend, but isn't
  --> /home/folkertdev/rust/rust/tests/ui/simd/intrinsic/generic-elements-pass.rs:88:17
   |
LL |         all_eq!(simd_insert_dyn(x2, 0, 100), i32x2([100, 21]));
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^


thread 'rustc' panicked at /home/folkertdev/rust/rust/compiler/rustc_codegen_ssa/src/mir/block.rs:987:29:

[workingjubilee]

odd.

[scottmcm]

is there some try job that would run tests/ui/simd/intrinsic/generic-elements-pass.rs with the fallback (e.g. with miri?)

It looks like something you could make const fn, right?

If so, you can test the fallback in CTFE. (Might need #[miri::intrinsic_fallback_is_spec]; I forget.)

[folkertdev]

none of that solves the actual problem of rustc somehow believing that this intrinsic must be overwritten by the backend. The only place where LLVM matches on the "simd_" prefix, I've disabled. But renaming to dyn_simd_{extract, insert} also does not fix the issue.

I can mark the functions as const (though extract needs to do a forget on the vector, which may or may not be correct). But even if that fixes the situation for miri, it does not for gcc and cranelift.

[folkertdev]

right, with the standard imports it's now hitting the LLVM codegen test, hence the fallback is actually used (but generates something different from what the test expects).

So, that should be sufficient right? Then the logic that prevents the LLVM implementation from being used can be removed and the hard work here should be done.

[rustbot]

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 to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

[RalfJung]

is there some try job that would run tests/ui/simd/intrinsic/generic-elements-pass.rs with the fallback (e.g. with miri?)

No, Miri has its own test suite.

If so, you can test the fallback in CTFE. (Might need #[miri::intrinsic_fallback_is_spec]; I forget.)

You're mixing up a few things here.^^ CTFE doesn't care about #[miri::intrinsic_fallback_is_spec], only Miri does. CTEF also doesn't implement most of the SIMD intrinsics.

If you add that attribute, note that it has a semantic meaning! It means the fallback must fully cover every case of the spec, including UB and non-determinism.

What would the fallback implementation even be here, given that all the types are unknown?

[RalfJung]

What would the fallback implementation even be here, given that all the types are unknown?

Ah, I have found the old implementations in the diffs.

unsafe { (&mut x as *mut T as *mut U).add(idx as usize).write(val) }

Adding intrinsic_fallback_is_spec here would be not correct! If the idx is out-of-bounds, this will not reliably trigger UB, instead it might override data that's outside the index but still inbounds of some allocation.

IOW, it would not be correct to specify the intrinsic as (&mut x as *mut T as *mut U).add(idx as usize).write(val), and therefore saying "the fallback implementation is the spec" would be wrong.

[workingjubilee]

Adding intrinsic_fallback_is_spec here would be not correct! If the idx is out-of-bounds, this will not reliably trigger UB, instead it might override data that's outside the index but still inbounds of some allocation.

How? The allocation would be the SIMD type itself, wouldn't it? Since that moves by-value into the function.

[workingjubilee]

I suppose it needs a copy bound, but

[RalfJung]

How? The allocation would be the SIMD type itself, wouldn't it? Since that moves by-value into the function.

Ah, fair, I forgot that this is not a reference/pointer.

[folkertdev]

not 100% following here: should I add #[miri::intrinsic_fallback_is_spec] to the new functions?

[RalfJung]

I think I would prefer a proper implementation in Miri. That should be trivial as the implementation is the same as the non-dyn variants. Will provide some links when I am back at a keyboard.

[RalfJung]

As far as I can tell, the implementation should be a trivial matter of adding | syn::simd_insert_dyn here

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

Line 406 in 788a389

sym::simd_insert => {

and the same with simd_extract_dyn a few lines down.

[folkertdev]

well there is some related discussion over on zullip on whether this is the best approach

#project-portable-simd > simd extraction/insertion with a non-`const` index

I think we've sort of settled on this being the best approach at the moment, but idk maybe it needs more discussion still.

[folkertdev]

well I believe this is ready and there's not been any further discussion I think this just needs review at this point

@rustbot ready

[scottmcm]

I think this is mostly fine, but did have some requested cleanup that I think would be good.

[folkertdev]

@rustbot ready

[scottmcm]

You probably still want

Suggested change

	//@compile-flags: -C no-prepopulate-passes -Z merge-functions=disabled
	//@compile-flags: -C opt-level=3 -C no-prepopulate-passes

so that rust always treats it as optimized, emitting things like noundef parameter attributes, so that (as you say) you get consistent output no matter the default optimization level.

-C no-prepopulate-passes just turns off the LLVM passes so that it doesn't change what rustc emitted.

[scottmcm]

(Which also means it turns off the function merging pass you so you don't need to worry about -Z merge-function.)

[folkertdev]

Got it, fixed that

☃

[scottmcm]

@bors r+

[bors]

📌 Commit 59c5533 has been approved by scottmcm

It is now in the queue for this repository.