std::arch SIMD intrinsics

[gnzlbg]

Currently the SIMD intrinsics are implemented in stdsimd using link_llvm_intrinsics to directly call the llvm intrinsics via their C ABI, and using a handful of "generic" simd intrinsics.

Is there a way to directly call Cretonne intrinsics?
Is there a cfg() macro available to detect whether the codegen backend is LLVM or Cranelift ?

[bjorn3]

Is there a way to directly call Cretonne intrinsics?

Cranelift doesn't have intrinsics. It does have simd types, which can be used with normal instructiosn like iadd. However I dont think all simd intrinsics have a cranelift instruction counterpart and implementing all of them takes time I would rather use to implement other things.

Is there a cfg() macro available to detect whether the codegen backend is LLVM or Cranelift?

Not yet, have been thinking about adding one though.

[gnzlbg]

@bjorn3 check this out:

https://github.com/rust-lang-nursery/stdsimd/blob/master/coresimd/x86/sse.rs#L1986

This is how std::arch calls into the different simd instructions (not all of them, but many of them). The question is, would it be possible to write similar code to target Cranelift ? Or should we move all of these into platform-intrinsics and add the abstraction layer at the Rust codegen level ?

It does have simd types, which can be used with normal instructiosn like iadd

That sounds more like what packed_simd does, which uses some of rustc's generic simd intrinsics, e.g., see here: https://github.com/rust-lang-nursery/packed_simd/blob/master/src/codegen/llvm.rs

It might be easier to get packed_simd to work with Cranelift than to get std::arch working, but note that packed_simd also uses std::arch intrinsics in many cases to work arounds codegen bugs in LLVM... A cfg macro to detect the backend would be needed here to detect Cranelift and remove these workarounds, but we might have to potentially add other workarounds for Cranelift, by "somehow" calling Cranelift SIMD operations.

---

Even if this work is not there yet, I think work to "prepare" std::arch and packed_simdfor Cranelift can already start, and ideally using such a cfg macro we would get one single std::arch intrinsic first, write down the process to "support Cranelift", and try to get people involved, mentor them, etc.

Removing link_llvm_intrinsics from std::arch would be a bit of work, but it is possible. And there are python generators in Rust upstream that generate platform-intrinsics for these "automatically". Maybe porting those to Cranelift would be a way forward. We can then use cfg macros in std::arch to only expose the simd instructions that Cranelift supports, and can work on adding support to Cranelift for more simd instructions until we reach parity.

[bjorn3]

cg_clif currently puts non-primitives in stackslots which would kill simd performance.

[bjorn3]

The question is, would it be possible to write similar code to target Cranelift ?

Not without changes to cg_clif to intercept those intrinsics.

Even if this work is not there yet, I think work to "prepare" std::arch and packed_simdfor Cranelift can already start, and ideally using such a cfg macro we would get one single std::arch intrinsic first, write down the process to "support Cranelift", and try to get people involved, mentor them, etc.

+1

[gnzlbg]

What would be desirable to start preparing stdsimd and packed_simd, is to clarify how the result should look like in those crates. cc @eddyb @sunfishcode

@eddyb was of the strong opinion that stdsimd should stop using link_llvm_intrinsics and start using platform intrinsics instead, but not all platform-intrinsics might be available for cranelift at least initially, so having some #[cfg(rustc_backend_llvm)] and #[cfg(rustc_backend_cranelift)] macros behind a feature gate in rustc would be useful for that, and also, to gate the llvm-specific workarounds of packed_simd.

The lowest-hanging fruit is probably to get packed_simd to work with cranelift, since many crates using std::arch also offer a packed_simd version behind a cargo feature to make their crate portable (e.g. rand). If we put all llvm workarounds behind feature gates in packed_simd, we would "only" have to implement the ~20 generic SIMD intrinsics for the cranelift backend.

[eddyb]

I don't think littering stdsimd with #[cfg]s is a good idea - the proper solution is to allow mixing LLVM and Cranelift codegen units like @sunfishcode suggested, and long-term have something like platform-intrinsics built into Cranelift, used as the source of truth (many of the instructions can be dealt with in a compact declarative fashion), with a mapping to LLVM names as a secondary interface.

[bjorn3]

Using platform-intrinsics to get packed_simd working seems doable. The simd_reduce_* family doesn't seem to have cranelift counterparts though. (cc @sunfishcode)

[bjorn3]

the proper solution is to allow mixing LLVM and Cranelift codegen units like @sunfishcode suggested

I like the idea. There is currently an abi incompatibility between cg_clif and cg_llvm. (cg_clif always passes non primitives by-ref and uses the cranelift fast calling convention instead of System-V like cg_llvm) Other than that it kind of works already today. (metadata is put in the same place and symbol names are made the same way)

[gnzlbg]

The simd_reduce_* family doesn't seem to have cranelift counterparts though.

I am not sure if these are necessary for an MVP of packed_simd working with cranelift or not. Maybe we could workaround these in cranelift, at least initially (e.g. by falling back to scalar code).

[eddyb]

Yeah I suspect any upstreamed backend to use e.g. FnType and strictly adhere to the ABI.
We can even come up with a Cranelift-friendly ABI that we use for LLVM too - we just have to be consistent and do everything through FnType.

[bjorn3]

I don't understand what to do when getting PassMode::Cast.

[eddyb]

You're supposed to pass the type's bytes as one or more immediates, as indicated by the information in the cast (e.g. rustc_target::abi::call::Reg tells you the register kind and size).

[bjorn3]

Got it. Thanks!

[bjorn3]

I implemented support for some simd_* intrinsics on the simd_emulation branch.

[gnzlbg]

I suppose that doing a scalar emulation might be initially ok, but doesn't cranelift support emitting the appropriate instructions?

[bjorn3]

Yes, it does for most intrinsics, but cranelift is currently implementing real simd, instead of emulation like I did here. (bytecodealliance/cranelift#833, bytecodealliance/cranelift#855, bytecodealliance/cranelift#868) Because of this I think for example adding two vectors is broken. (cc @abrown, am I correct?) Also using real SIMD will not give much performance benifit yet, until some changes to the rest of cg_clif to not always store the vectors on the stack are performed and inlining of the std::arch functions is performed.

[abrown]

Because of this I think for example adding two vectors is broken

I think this is only broken if you turn on the enable_simd setting because only a handful of SIMD instructions are implemented. Otherwise, the vectors are split up and--as far as I understand cranelift--the vector addition should work (see https://github.com/CraneStation/cranelift/blob/a5b17e8a0f044ec03b6982baea3757af43e70b7b/cranelift-codegen/src/isa/x86/abi.rs#L87-L95). More SIMD instructions are coming but we need to review and merge foundational stuff like bytecodealliance/cranelift#855 and bytecodealliance/cranelift#868 --any help is appreciated 😄.

[bjorn3]

I think this is only broken if you turn on the enable_simd setting

Of cource, forgot about that setting.

[bjorn3]

Opened #650.

[bjorn3]

https://github.com/bjorn3/rustc_codegen_cranelift/pull/1378 and https://github.com/bjorn3/rustc_codegen_cranelift/pull/1380 implemented a bunch more intrinsics.

[bjorn3]

Enough intrinsics are supported now that it seems like removing the hack to make is_x86_feature_detected!() return false in https://github.com/bjorn3/rustc_codegen_cranelift/pull/1397 didn't cause issues for anyone. Or at least nobody reported an issue because of this change.

[bjorn3]

Rav1e and image now thanks to e5ba1e8 and a558968 respectively.

[bjorn3]

#1417 implemented a lot of intrinsics that were found to be missing.

[benwis]

So I'm testing cranelift a bit in our group, so far we've hit these missing instructions. Not sure there's a workaround for that right now. Also curious how people figure out what dep is calling these, and how to best track when PRs land in rust nightly

core::arch::x86::avx::_mm256_ldqu_si256 llvm.x86.avx.ldu.dq.256
llvm.x86.aesni.aesenc
llvm.x86.aesni.aesimc
llvm.x86.aesni.aesenclast

I think the first one might be fixed by #1417. Loving cranelift where we can use this though!

[bjorn3]

Also curious how people figure out what dep is calling these

I just get a backtrace at the crash site using a debugger.

and how to best track when PRs land in rust nightly

I will post a comment in the respective issue once it lands on nightly.

Not sure there's a workaround for that right now.

For the aes intrinsics you can use RUSTFLAGS="--cfg aes_force_soft" when compiling as workaround to force the aes crate to use a software implementation instead of the simd intrinsics.

[AppleSheeple]

Also curious how people figure out what dep is calling these

I just get a backtrace at the crash site using a debugger.

FWIW, I went with a simple cargo vendor and grep approach, which lead to #1410.

This should be simpler, as it doesn't burden you with running the code and trying to hit the traps. And also, there could be more than one dependency using the same missing intrinsics reported, so run and debug may miss some usage cases in dependencies.

[pothos]

For making the codegen production-ready I think it's important to fail compilation when an unsupported intrinsic is hit. The replacement with a trap should be opt-in (probably only through a debug env var) for a safe behaviour. Think about not noticing a regression from a crate update in a build/release pipeline because there is just this warning instead of failing compilation hard.

Edit: Also, IDE users might easily miss the warning if the IDE triggers the compilation in the background.

[bjorn3]

Rustc_codegen_cranelift is meant for use during development. For release builds I recommend the default LLVM backend or in the future the GCC backend as both produce much faster executables. In the future I agree that it makes sense to turn this into a hard error by default, but until there are enough simd intrinsics implemented for that to rarely result in a compile error, I did rather keep it as warning + trap at runtime.

[bjorn3]

#1443 by @Nilstrieb implemented a couple of x86 pack intrinsics and fixed a couple other of these intrinsics.