Implement RFC 2574, "SIMD vectors in FFI" #86546

JohnTitor · 2021-06-22T11:29:27Z

Re-submission of #59238, cc #63068

Remaining tasks:

~~[WIP] Implement RFC2574 for FFI declarations #59238 (comment)~~ splitting to future-incompat: use of SIMD types aren't gated properly #87438
- ~~I'm not sure if it's really addressed on this PR, should we?~~
[WIP] Implement RFC2574 for FFI declarations #59238 (comment)
- add codegen (assembly) tests
- 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 (investigate if it's still a problem)

rust-highfive · 2021-06-22T11:29:30Z

r? @varkor

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

compiler/rustc_typeck/src/collect.rs

varkor · 2021-06-30T21:54:28Z

Sorry, I don't have time to review this right now. Reassigning arbitrarily to r? @oli-obk

JohnTitor · 2021-07-01T17:35:42Z

@varkor FWIW you could remove yourself from the review rotation if you'd like: https://github.com/rust-lang/highfive/blob/2d1fdf4d7321863060935c06fc7788c1081679e8/highfive/configs/rust-lang/rust.json#L8
(I've heard you're going to focus on the PhD studies, I wish best of luck with it :))

varkor · 2021-07-01T18:08:17Z

@JohnTitor: I can find time to review smaller PRs (e.g. < 100 lines changed), which is why I haven't removed myself at the moment, but I have been auto-assigned quite a lot of larger ones recently; I may take your advice if I find it is too overwhelming :)

oli-obk

I'm not sure if it's really addressed on this PR, should we?

since it already works on stable, we would have to make it a future incompat lint. Since it is already on stable, there's no hurry, it's not like this PR suddenly allows that, right? (if this PR allows that on stable, then yes, it needs to be fixed) So you can just open an issue for it.

oli-obk · 2021-07-05T16:32:05Z

compiler/rustc_typeck/src/collect.rs

+/// Returns `Ok()` if the target-feature allows using the SIMD type on C FFI.
+/// Otherwise, returns `Err(Some())` if the target_feature needs to be enabled or
+/// or `Err(None)` if it's unsupported.
+fn simd_ffi_feature_check(


I don't know enough about simd to properly review this function. @lqd can you help or ping others?

That said, the function body looks very fragile and easy to get accidentally wrong to me. Is there something declarative that we could parse and process here?

I can help ... by asking the 2 greats @Amanieu and @nagisa for help ^^

That said, the function body looks very fragile and easy to get accidentally wrong to me. Is there something declarative that we could parse and process here?

I agree but I haven't come up with/know a better solution. @Amanieu Do you have any ideas?

The true source of these ABI constraints is in LLVM's handling of calling conventions. I don't think there is anything we can easily parse here.

My recommendation would be to directly refer to LLVM's source code for calling convention handling to determine which target features affect ABI.

In a way this is similar to what I had to do for asm! in compiler/rustc_target/src/asm which is similarly tied to LLVM implementation details for reserved registers.

Couldn't LLVM be extended to provide APIs to query information about calling conventions instead of relying on its source code, which is also fragile. LLVM is huge database. Or maybe use tablegen to create rust files ...

tschuett · 2021-07-06T19:04:11Z

compiler/rustc_typeck/src/collect.rs

+            // * on mips: 16 => msa,
+            // * wasm: 16 => simd128
+            match simd_width {
+                8 if feature.contains("mmx")


Maybe instead of match:

if feature.contains("mmx") and simd_with == 8 { return Ok(()); } if feature.contains("see") and 8..=16.contains(simd_with) { return Ok(()); } ... if feature.contains("avx512") and 8..=64.contains(simd_width) { return Ok(()); }

Hmm, I'm not sure if it's much improvement, at least the current is readable enough to me. And I feel like that it makes it slightly harder to understand that what 8, 16, 32, ... mean.

compiler/rustc_typeck/src/collect.rs

JohnTitor · 2021-07-24T17:55:28Z

since it already works on stable, we would have to make it a future incompat lint. Since it is already on stable, there's no hurry, it's not like this PR suddenly allows that, right? (if this PR allows that on stable, then yes, it needs to be fixed) So you can just open an issue for it.

Yes, we can do it separately, I'm going to open a new issue. Thanks!

EDIT: Opened #87438.

bors · 2021-08-24T06:53:27Z

☔ The latest upstream changes (presumably #87739) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2021-10-31T07:44:02Z

Ping from triage:
@JohnTitor - can you please post your status on this PR? You have a merge conflict. Is this abandoned?

Dylan-DPC · 2022-02-19T02:18:36Z

@JohnTitor any updates on this?

JohnCSimon · 2022-03-20T05:45:39Z

@JohnTitor
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thanks for your contribution.

@rustbot label: +S-inactive

rust-highfive assigned varkor Jun 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2021

This comment has been minimized.

Sign in to view

tschuett reviewed Jun 22, 2021

View reviewed changes

compiler/rustc_typeck/src/collect.rs Show resolved Hide resolved

JohnTitor force-pushed the simd-ffi branch from fd2067d to 9bfe61e Compare June 28, 2021 22:56

rust-highfive assigned oli-obk and unassigned varkor Jun 30, 2021

oli-obk reviewed Jul 5, 2021

View reviewed changes

tschuett reviewed Jul 6, 2021

View reviewed changes

Amanieu reviewed Jul 7, 2021

View reviewed changes

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved

crlf0710 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 Jul 24, 2021

JohnTitor added 12 commits July 25, 2021 02:30

Add some basic tests

22e8f1e

Allow to apply #[target_fature] to foreign items

4a22c0a

Implement SIMD FFI checks

f387030

Fix feature gate check

d5471a7

Only allow foreign functions for SIMD FFI

4702a9a

Fix cycle error

ad10322

Catch incorrect uses of SIMD vectors

d617ed4

Add FXIME test

79ac2dc

Bless tests

2360446

Fix a test

564779c

Add some notes about arguments

eb9ed3a

Address review comments

5002006

JohnTitor force-pushed the simd-ffi branch from 9bfe61e to 5002006 Compare July 24, 2021 17:53

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

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

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

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

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

JohnCSimon closed this Mar 20, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 20, 2022

JohnTitor mentioned this pull request Apr 14, 2022

Tracking issue for RFC 2574, "SIMD vectors in FFI" #63068

Open

4 tasks

JohnTitor deleted the simd-ffi branch June 25, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 2574, "SIMD vectors in FFI" #86546

Implement RFC 2574, "SIMD vectors in FFI" #86546

JohnTitor commented Jun 22, 2021 •

edited

Loading

rust-highfive commented Jun 22, 2021

This comment has been minimized.

varkor commented Jun 30, 2021

JohnTitor commented Jul 1, 2021

varkor commented Jul 1, 2021

oli-obk left a comment

oli-obk Jul 5, 2021

lqd Jul 6, 2021

JohnTitor Jul 24, 2021

Amanieu Jul 24, 2021

tschuett Jul 25, 2021 •

edited

Loading

tschuett Jul 6, 2021

JohnTitor Jul 24, 2021 •

edited

Loading

JohnTitor commented Jul 24, 2021 •

edited

Loading

bors commented Aug 24, 2021

JohnCSimon commented Oct 31, 2021

Dylan-DPC commented Feb 19, 2022

JohnCSimon commented Mar 20, 2022

Implement RFC 2574, "SIMD vectors in FFI" #86546

Implement RFC 2574, "SIMD vectors in FFI" #86546

Conversation

JohnTitor commented Jun 22, 2021 • edited Loading

rust-highfive commented Jun 22, 2021

This comment has been minimized.

varkor commented Jun 30, 2021

JohnTitor commented Jul 1, 2021

varkor commented Jul 1, 2021

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Jul 5, 2021

Choose a reason for hiding this comment

lqd Jul 6, 2021

Choose a reason for hiding this comment

JohnTitor Jul 24, 2021

Choose a reason for hiding this comment

Amanieu Jul 24, 2021

Choose a reason for hiding this comment

tschuett Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

tschuett Jul 6, 2021

Choose a reason for hiding this comment

JohnTitor Jul 24, 2021 • edited Loading

Choose a reason for hiding this comment

JohnTitor commented Jul 24, 2021 • edited Loading

bors commented Aug 24, 2021

JohnCSimon commented Oct 31, 2021

Dylan-DPC commented Feb 19, 2022

JohnCSimon commented Mar 20, 2022

JohnTitor commented Jun 22, 2021 •

edited

Loading

tschuett Jul 25, 2021 •

edited

Loading

JohnTitor Jul 24, 2021 •

edited

Loading

JohnTitor commented Jul 24, 2021 •

edited

Loading