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

Stabilize ADX, TBM, and SSE4a target features #60109

Closed
wants to merge 5 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Apr 19, 2019

This PR stabilizes the ADX, TBM, and SSE4a target features.

We stabilized the ADX core::arch intrinsics in Rust 1.33, and the TBM and SSE4a intrinsics have been accidentally stabilized since Rust 1.27. AFAICT there is no good reason to leave those as stabilized (we got lucky I guess), and while we could unstabilize them, it seems that they are worth stabilizing right after, so that would just introduce unnecessary churn.

It seems that when we stabilized the ADX intrinsics, we just forgot to also stabilize the ADX target feature attribute.

r? @alexcrichton

cc @jethrogb @Centril @rust-lang/libs (for the decision of not unstabilizing the accidentally stabilized intrinsics) @rust-lang/lang (for the decision of stabilizing the #[target_feature(enable = "adx,sse4a,tbm")] target features).

cc #44839 - tracking issue for target feature

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 19, 2019

The ui tests will probably need updating. Also, I'm not sure if Rust 1.35.0 is the right version to use here.

@rust-highfive

This comment has been minimized.

@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 19, 2019
@Centril
Copy link
Contributor

Centril commented Apr 19, 2019

@gnzlbg Looks good, tho the docs of https://doc.rust-lang.org/nightly/core/arch/x86_64/fn._mm_stream_ss.html could be a bit more illuminating for the unfamiliar but curious reader. MSDN provides more elaboration: https://docs.microsoft.com/en-us/cpp/intrinsics/mm-stream-ss?view=vs-2019.

An idea for the future: Can we feasibly link to the relevant vendor documentation for each function, e.g. Intel's docs for a specific intrinsic?

Also, the current nightly is 1.36 so 1.35 seems wrong?

@alexcrichton shall we start FCP?

@alexcrichton
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 19, 2019

Team member @alexcrichton 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!

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 19, 2019
@jethrogb
Copy link
Contributor

Can we feasibly link to the relevant vendor documentation for each function, e.g. Intel's docs for a specific intrinsic?

Not really. Intel provides two types of documentation: the intrinsics guide, which is often not very informative, and the software developer's manual, which is a many-1000 page PDF document, so it's not easy to link to.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 20, 2019

Agree with @jethrogb . In this case, SSE4a is not an Intel intrinsic, but an AMD one, and AMD does the exact same thing (1000s of pages long pdfs). These pdfs are not designed with tools that want to parse them in mind, so I don't think it's a feasible thing to do.

The Intel Intrinsics guide is available in XML format, and we do parse that for automatic verification of the intrinsics names/type signatures/instructions/etc. but the docs in there are not very good. The clang docs are better, and the MSVC docs are even better (they often contain working examples for each intrinsic), so maybe we could automatically parse those and use them to generate documentation (at least under the assumption that we could sort out licensing issues).

If somebody wants to work on doing this I'll be very happy to mentor.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 20, 2019

Also, the current nightly is 1.36 so 1.35 seems wrong?

I'll fix the tests and set this to 1.36.0!

@Centril Centril added this to the 1.36 milestone Apr 22, 2019
@bors
Copy link
Contributor

bors commented Apr 30, 2019

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

@nikomatsakis
Copy link
Contributor

NB: I checked @aturon's box as he is presently on leave.

@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 9, 2019
@rfcbot
Copy link

rfcbot commented May 9, 2019

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

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label May 19, 2019
@rfcbot
Copy link

rfcbot commented May 19, 2019

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.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 19, 2019
@alexcrichton
Copy link
Member

@gnzlbg want to rebase and I'll r+?

@Centril Centril 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 May 27, 2019
@Centril Centril modified the milestones: 1.36, 1.37 May 27, 2019
@Centril
Copy link
Contributor

Centril commented May 27, 2019

When you rebase, remember to adjust the stabilization dates to 1.37.0.

@gnzlbg gnzlbg force-pushed the stabilize_target_features branch from f6059b1 to 672d0e6 Compare June 22, 2019 14:22
@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019
@mati865
Copy link
Contributor

mati865 commented Jul 19, 2019

This has slipped to 1.38 and there is a conflict.

@wirelessringo
Copy link

Ping from triage @gnzlbg any updates on this?

@JohnTitor
Copy link
Member

Ping from triage: @gnzlbg, you should resolve conflict and bump up the target version to 1.38.

@Centril Centril modified the milestones: 1.38, 1.39 Aug 13, 2019
@JohnTitor
Copy link
Member

Ping from triage: @gnzlbg, you should resolve conflicts and bump up the target version to 1.39.

@hdhoang
Copy link
Contributor

hdhoang commented Sep 6, 2019

Ping from triage @gnzlbg, thank you for your work on this, please re-open this PR once you can resolve the conflicts (please re-open first then push).

@hdhoang hdhoang closed this Sep 6, 2019
@hdhoang hdhoang added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2019
@dnrusakov
Copy link

@gnzlbg Are you still planning to finish this work?

@Centril Centril removed this from the 1.39 milestone Sep 26, 2019
tarcieri added a commit to tarcieri/rust that referenced this pull request Mar 15, 2022
This is a continuation of rust-lang#60109, which noted that while the ADX
intrinsics were stabilized, the corresponding target feature never was.

This PR follows the same general structure and stabilizes the ADX target
feature.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 17, 2022
Stabilize ADX target feature

This is a continuation of rust-lang#60109, which noted that while the ADX intrinsics were stabilized, the corresponding target feature never was.

This PR follows the same general structure and stabilizes the ADX target feature.

See also rust-lang#44839 - tracking issue for target feature
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. relnotes Marks issues that should be documented in the release notes of the next release. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.