Skip to content

make abi_unsupported_vector_types a hard error #139309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make abi_unsupported_vector_types a hard error #139309

wants to merge 1 commit into from
+124 −955

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 3, 2025 •
edited
Loading

Fixes #116558 by completing the transition; see that issue for context. The lint was introduced with Rust 1.84 and this has been shown in cargo's future breakage reports since Rust 1.85, released 6 weeks ago, and so far we got 0 complaints by users. There's not even a backlink on the tracking issue. We did a crater run when the lint was originally added and found no breakage. So I don't think we need another crater run now, but I can do one if the team prefers that.

#131800 is done, so for most current targets (in particular, all tier 1 and tier 2 targets) we have the information to implement this check (modulo the targets where we don't properly support SIMD vectors yet, see the sub-issues of #116558). If a new target gets added in the future, it will default to reject all SIMD vector types until proper information is added, which is the default we want.

This will need approval by for @rust-lang/lang. Cc @workingjubilee @veluca93

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

r? @fee1-dead

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 3, 2025
@RalfJung RalfJung added I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels Apr 3, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from f4e41c2 to 607bd91 Compare April 3, 2025 10:30
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from 607bd91 to 39f82c8 Compare April 3, 2025 11:31
RalfJung
RalfJung commented Apr 3, 2025
View reviewed changes
tests/codegen/regparm-inreg.rs
@@ -3,7 +3,7 @@
// x86 only.

//@ add-core-stubs
//@ compile-flags: --target i686-unknown-linux-gnu -Cno-prepopulate-passes -Copt-level=3
//@ compile-flags: --target i686-unknown-linux-gnu -Cno-prepopulate-passes -Copt-level=3 -Ctarget-feature=+avx
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azhogin this test was using SIMD vector types without the appropriate target feature... I hope adding the target feature preserves the meaning of the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from 39f82c8 to d817e10 Compare April 3, 2025 12:41
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from d817e10 to 00743be Compare April 3, 2025 13:45
@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from 00743be to 764fe74 Compare April 3, 2025 14:51
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Apr 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

This PR modifies run-make tests.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from 764fe74 to f6eb25c Compare April 3, 2025 17:04
RalfJung
RalfJung commented Apr 3, 2025
View reviewed changes
tests/run-make/simd-ffi/rmake.rs
@@ -56,7 +56,7 @@ fn main() {
.target(&target)
.emit("llvm-ir,asm")
.input("simd.rs")
.arg("-Ctarget-feature=-soft-float,+neon,+sse")
.arg("-Ctarget-feature=-soft-float,+neon,+sse2,+msa")
.arg(&format!("-Cextra-filename=-{target}"))
.run();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will become a complete nightmare to maintain when we start to properly enforce some basic consistency of target features.

@ehuss
Copy link
Contributor

ehuss commented Apr 3, 2025

@RalfJung Do you think we can get this documented in the reference? Like maybe somewhere in the target_feature documentation or probably it makes more sense to be a part of the abi chapter with the abi chapter rework? Although I'm a little confused since Connor removed the SIMD section and I'm not sure why.

My uncertainty is emphasized because we currently don't really document SIMD, and I don't know how we plan to do that.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung force-pushed the abi_unsupported_vector_types branch from f6eb25c to 5bbf200 Compare April 4, 2025 06:01
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2025

@RalfJung Do you think we can get this documented in the reference? Like maybe somewhere in the target_feature documentation or probably it makes more sense to be a part of the abi chapter with the abi chapter rework? Although I'm a little confused since Connor removed the SIMD section and I'm not sure why.

My uncertainty is emphasized because we currently don't really document SIMD, and I don't know how we plan to do that.

Seems like this should be part of rust-lang/reference#1545 so I'd rather not add a separate PR now, that would just be a huge conflict.

@ehuss ehuss mentioned this pull request Apr 4, 2025
Open
@bors
Copy link
Collaborator

bors commented Apr 9, 2025

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

fee1-dead
fee1-dead approved these changes Apr 9, 2025
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler changes LGTM

@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 9, 2025
@tmandry
Copy link
Member

tmandry commented Apr 9, 2025

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 9, 2025 •
edited by joshtriplett
Loading

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 9, 2025
@traviscross
Copy link
Contributor

Sounds right to me.

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 9, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 9, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@fee1-dead fee1-dead added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2025
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 16, 2025
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@workingjubilee workingjubilee workingjubilee left review comments

@fee1-dead fee1-dead fee1-dead approved these changes

Assignees

@fee1-dead fee1-dead

Labels
A-run-make Area: port run-make Makefiles to rmake.rs disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone