Skip to content
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.

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

Remove misleading target feature aliases #107707

Merged
merged 1 commit into from
May 15, 2023

Conversation

calebzulawski
Copy link
Member

Fixes #100752. This is a follow up to #103750. These aliases could not be completely removed until rust-lang/stdarch#1355 landed.

cc @Amanieu

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@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 Feb 5, 2023
@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2023

LGTM, but I believe this needs T-lang signoff since it affects target features. However I do believe these features are still unstable so it should be fine.

@calebzulawski
Copy link
Member Author

Yep, they are still unstable.
r? @rust-lang/lang

@rustbot rustbot assigned joshtriplett and unassigned davidtwco Feb 6, 2023
@workingjubilee
Copy link
Member

For context for T-lang, since it might be a bit cryptic from the remarks elsewhere:

There are in fact "avx512{gfni, vaes, ...}" instructions and so on, but the x86 CPU denotes that these instructions are available by signaling "both the avx512 bit and the feature bit enabled" when CPUID is queried. For each feature, it signals that "only" the "feature" instructions are available via having the feature bit enabled but the avx512 bit disabled. The exact details of how this works out sometimes varies a little from what I just said since x86 is a very... "evolved, not designed" instruction set architecture.

@anden3 anden3 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 Apr 13, 2023
@anden3
Copy link
Contributor

anden3 commented Apr 13, 2023

Hi @calebzulawski! What's the status on this PR? I saw you got a review on the 6th of February :)

@Amanieu
Copy link
Member

Amanieu commented Apr 13, 2023

This is waiting on T-lang sign-off. Otherwise the PR itself looks fine.

@anden3 anden3 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2023
@nikomatsakis
Copy link
Contributor

If you want lang team signoff, we need to nominate it for lang-team discussion! See instructions here:

https://lang-team.rust-lang.org/how_to/nominate.html

In particular, can you author a relatively standalone paragraph explaining the question to be answered? Thanks!

@calebzulawski
Copy link
Member Author

@rustbot label +I-lang-nominated

When AVX512 support was initially added, these three features (GFNI, VAES, and VPCLMULQDQ) were included as AVX512 features and prefixed with the avx512 name. These features are not yet stabilized.

These features do provide AVX512 instructions when paired with the avx512f feature, but on their own they do not. This is not just theoretical--some Intel CPUs support e.g. GFNI that only operates on SSE and AVX vectors (without any AVX512 support). Intel (and LLVM) do not include AVX512 in the names of these features, either. Having the avx512 prefix may mislead a user into believing the avx512f feature is enabled by these features, which is true for all other AVX512 features.

My previous PR added the more accurate gfni, vaes, and vpclmulqdq target feature names. This PR introduces a nightly/unstable breaking change by removing the avx512gfni, avx512vaes, and avx512vpclmulqdq aliases for these features.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 22, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Discussed in @rust-lang/lang meeting. We feel good about this change, particularly since target feature gives you warnings if things are wrong.

@rfcbot

This comment was marked as outdated.

@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 May 2, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented May 2, 2023

@nikomatsakis proposal cancelled.

@rfcbot rfcbot removed 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 May 2, 2023
@nikomatsakis nikomatsakis 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 May 2, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Discussed in https://github.com/orgs/rust-lang/teams/lang meeting. We feel good about this change, particularly since target feature gives you warnings if things are wrong.

@rfcbot
Copy link

rfcbot commented May 2, 2023

Team member @nikomatsakis 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 May 2, 2023
@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 2, 2023
@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 May 2, 2023
@rfcbot
Copy link

rfcbot commented May 2, 2023

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

@nikomatsakis
Copy link
Contributor

@rustbot labels -S-waiting-on-team

@rustbot rustbot removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 9, 2023
@nikomatsakis
Copy link
Contributor

@rustbot labels -I-lang-nominated

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label May 9, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 12, 2023
@rfcbot
Copy link

rfcbot commented May 12, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@Amanieu
Copy link
Member

Amanieu commented May 15, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2023

📌 Commit 47fc132 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 15, 2023
@bors
Copy link
Contributor

bors commented May 15, 2023

⌛ Testing commit 47fc132 with merge ce5919f...

@bors
Copy link
Contributor

bors commented May 15, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing ce5919f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 15, 2023
@bors bors merged commit ce5919f into rust-lang:master May 15, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 15, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ce5919f): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.9% [-1.9%, -1.9%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.425s -> 644.092s (-0.05%)

@calebzulawski calebzulawski deleted the remove-features branch May 15, 2023 23:43
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider renaming the avx512gfni feature to just gfni