[WIP] Implement RFC2574 for FFI declarations #59238
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Looking pretty good to me!
One thing I'm curious about, you've got a bit of checking here for something like how any number of 3 different features could enable 128-bit SIMD, but in reality I think anything above SSE enables 128-bit SIMD, right? (or something like that). Do we want to eventually support all this? Support where a feature enables all its hierarchichal features as well, and only check for the bare minimum?
I was thinking it may be best to start conservatively and just say that if you use 128-bit vectors you must say "feature x is enabled" where "x" is defined per architecture.
Additionally I'm not sure if we actually respect target_feature on FFI definitions, but can you verify? We'd want to verify that we emit the right LLVM target feature and then LLVM actually does pass the argument in a vector register.
So the RFC only requires this last thing. For example, if one want to use There was general consensus that we should try to do better here, but I didn't wanted to mix a discussion about x86 feature hierarchies in the RFC, yet this was my private branch and I wanted to figure out how hard could this be. As you have discovered, there are still some bugs open, e.g., enabling If these things are easy to solve, which appears to be the case, my intent is to make them part of the PR. If the logic ends up being to complicated, I think that you are right and we might want to do that in a subsequent PR.
Oh, nice catch! I don't know either. I'll just add a codegen test for this here and if it fails I'll just fix it as part of this PR if that's ok. |
|
Ok cool that makes sense to me. If you have access to |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| // ignore-mips | ||
| // ignore-mips64 | ||
| // ignore-aarch64 | ||
| // ignore-arm |
There was a problem hiding this comment.
I think nowadays we have // only-x86
| fn simd_ffi_check<'a, 'tcx, 'b: 'tcx>( | ||
| tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId, ast_ty: &hir::Ty, ty: Ty<'b>, | ||
| ) { | ||
| if !ty.is_simd() { |
There was a problem hiding this comment.
One thing that this won't handle I think is:
#[repr(transparent)]
struct MyType(__m128i);but is that perhaps best left for a future implementation?
There was a problem hiding this comment.
I'm also not even sure if this is something we properly gate today
There was a problem hiding this comment.
Nice catch, and no, we don't gate this today:
use std::arch::x86_64::__m128;
#[allow(improper_ctypes)]
extern "C" {
//fn e(x: __m128); // ERROR
pub fn a(x: A);
pub fn b(x: B);
}
#[repr(transparent)] pub struct A(__m128);
#[repr(C)] pub struct B(__m128);both of these are allowed on stable Rust, and both should be rejected.
There was a problem hiding this comment.
We also should gate foreign function definitions:
use std::arch::x86_64::__m128;
pub extern "C" fn foo(x: __m128) -> __m128 { x }These also work on stable Rust.
I'll try to fix all of these issues as part of this PR. We can do a crater run afterwards, and see if we need to change any of these to warn instead of error.
| }).unwrap().details.size.bytes() as usize; | ||
| let simd_elem_width = simd_len / ty.simd_size(tcx); | ||
| let target: &str = &tcx.sess.target.target.arch; | ||
| if !features.iter().any(|f| simd_ffi_feature_check( |
There was a problem hiding this comment.
Could this condition get inverted to return early to de-indent the error generation code?
| // * on mips: 16 => msa, | ||
| // * wasm: 16 => simd128 | ||
| match target { | ||
| t if t.contains("x86") => { |
There was a problem hiding this comment.
Would it be possible to deduplicate this block up here with the block down below?
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #59298) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Haven't had the chance to look at the implementation, but scanning over the tests I am concerned by the lack of codegen tests (more specifically, assembly tests as LLVM IR doesn't really tell you which registers are actually used) for whether the now-allowed FFI calls really generate the right code. For example, AFAIK we still don't generate the shim that's necessary to allow passing __m256 in ymm registers if the caller doesn't have AVX enabled themselves. Would be good to have an xfail test for that and a test that ensure everything works if the caller has AVX enabled. |
An assembly test suite has been recently introduced, so this sounds like a good idea to me.
Yeah, this PR needs to fix that: https://rust.godbolt.org/z/rYraGF |
|
ping from triage @gnzlbg @alexcrichton any updates on this? |
Dylan-DPC-zz
added
S-waiting-on-author
| @@ -0,0 +1,57 @@ | |||
| // ignore-tidy-linelength | |||
There was a problem hiding this comment.
Please move all tests that relate to simd_ffi into: ui/rfc-2574-simd-ffi/$file.
|
Ping from triage @gnzlbg, this has merge conflicts and review comments to be resolved |
|
Pinging from triage again @gnzlbg |
|
@gnzlbg Hello from triage. Unfortunately this hasn't seen any movement in a month. Closing due to inactivity. |
JohnTitor
added
S-inactive
This PR implements RFC2574 (rust-lang/rfcs#2574) for FFI declarations only. This is done by checking whether target features explicitly enabled for FFI declarations allow using the vector types that appear on the function signature.
WIP: I'm a bit stuck here. I'm getting the following error:
but I have no idea what this means. How can there be a cycle here? AFAICT the new functionality is not recursive. Is there a way to dump something more about the cycle ?