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

Updated the list of white-listed target features for x86 #78361

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

DevJPM
Copy link
Contributor

@DevJPM DevJPM commented Oct 25, 2020

This PR both adds in-source documentation on what to look out for when adding a new (X86) feature set and adds all that are detectable at run-time in Rust stable as of 1.27.0.

This should only enable the use of the corresponding LLVM intrinsics.
Actual intrinsics need to be added separately in rust-lang/stdarch.

It also re-orders the run-time-detect test statements to be more consistent
with the actual list of intrinsics whitelisted and removes underscores not present
in the actual names (which might be mistaken as being part of the name)

The reference for LLVM's feature names used is this file.

This PR was motivated as the compiler end's part for allowing #67329 to be adressed over on rust-lang/stdarch

This PR both adds in-source documentation on what to look out for
when adding a new (X86) feature set and adds all that are detectable at run-time in Rust stable
as of 1.27.0.

This should only enable the use of the corresponding LLVM intrinsics.
Actual intrinsics need to be added separately in rust-lang/stdarch.

It also re-orders the run-time-detect test statements to be more consistent
with the actual list of intrinsics whitelisted and removes underscores not present
in the actual names (which might be mistaken as being part of the name)
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2020
`movbe` seems to not be a run-time detectable feature on x86.
It has thus been removed from the list.
It was only commented out to ease comparison against the full list.
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/project-portable-simd (perhaps?)

I'm going to r? @KodrAus here though in the hopes that you're more familiar or can direct this to an appropriate reviewer

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Thank you for opening a PR for this much-needed update. Alas, it seems to be slightly overeager in trying to fix absolutely everything up all at once and has provoked some interesting errors from the test suite. They don't seem to be ones of a type that one can just ./x.py test --blessing away, either, but you can always try I suppose.

Comment on lines -61 to -62
println!("tsc: {:?}", is_x86_feature_detected!("tsc"));
println!("mmx: {:?}", is_x86_feature_detected!("mmx"));
Copy link
Member

Choose a reason for hiding this comment

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

It is not intended that MMX feature detection be removed from Rust, in spite of the fact we don't do anything with it. The detection of capabilities and emission of such code are two entirely different concerns. Likewise it is not clear why tsc is being removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removal (as well as the removal of the test for the abm detection) were unintentional. I have re-added them, though above the other features to preserve consistency with X86_ALLOWED_FEATURES.

@@ -137,13 +137,18 @@ pub fn time_trace_profiler_finish(file_name: &str) {
// WARNING: the features after applying `to_llvm_feature` must be known
// to LLVM or the feature detection code will walk past the end of the feature
// array, leading to crashes.
// To find a list of LLVM's names, check llvm-project/llvm/include/llvm/Support/*TargetParser.def
// where the * matches the architecture's name
Copy link
Member

Choose a reason for hiding this comment

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

You should remind people to check out the submodule in our repo and its current commit, e.g. currently https://github.com/rust-lang/llvm-project/tree/ee1617457899ef2eb55dcf7ee2758b4340b6533f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added that as well as a note that the git submodule isn't the whole truth but instead external supported LLVM versions need to also be considered - the internal one seems to be LLVM 11 (ish)

@@ -50,15 +55,23 @@ const X86_ALLOWED_FEATURES: &[(&str, Option<Symbol>)] = &[
("aes", None),
("avx", None),
("avx2", None),
("avx512bf16", Some(sym::avx512_target_feature)),
Copy link
Member

Choose a reason for hiding this comment

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

The tests are going absolutely bonkers over the absence of this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have commented this feature out, it seems the support was only added in LLVM 9 and apparently we still support LLVM 8 (if the name of the relevant CI check is to be believed and my interpretation of the CI is correct). Is there some tracking issue for the supported external LLVM version where I can follow to re-add this change once the base support LLVM version is bumped?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm? Not aware of any policy around that, sorry!

("avx512vl", Some(sym::avx512_target_feature)),
("avx512vnni", Some(sym::avx512_target_feature)),
("avx512vp2intersect", Some(sym::avx512_target_feature)),
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

…M 9 exclusive features

Updated the added documentation in llvm_util.rs to note which copies of LLVM need to be inspected.
Removed avx512bf16 and avx512vp2intersect because they are unsupported before LLVM 9 with the build with external LLVM 8 being supported
Re-introduced detection testing previously removed for un-requestable features tsc and mmx
@KodrAus
Copy link
Contributor

KodrAus commented Nov 6, 2020

r? @workingjubilee

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Ah! Everything looks good now, and that is an even better note than my suggestion, so if CI approves then so do I. But alas, given that it did not reassign the PR to me, I think you still have to sign off, @KodrAus :^)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 15, 2020
Bump minimal supported LLVM version to 9

This bumps the minimal tested llvm version to 9.
This should enable supporting newer LLVM features (and CPU extensions).

This was motived by rust-lang#78361 having to drop features because of LLVM 8 not supporting certain CPU extensions yet.
This was declared relatively uncontroversial on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Min.20Supported.20LLVM.20Upgrade.20Process.3F/near/215957859).

Paging ````@eddyb```` because there was a comment in the [dockerfile](https://github.com/rust-lang/rust/blob/master/src/ci/docker/host-x86_64/x86_64-gnu-llvm-8/Dockerfile#L42) describing a hack (which I don't quite understand) which was also blocked by not having LLVM 9.
With rust-lang#78848 merged, the minimum supported LLVM version is now 9
which means we can actually use the target features introduced in LLVM 9
@DevJPM
Copy link
Contributor Author

DevJPM commented Nov 15, 2020

Now that #78848 is merged, we can use the LLVM 9 target features here which is why I pushed them.

I think @workingjubilee still wants @KodrAus to sign off on this PR?

@KodrAus
Copy link
Contributor

KodrAus commented Nov 15, 2020

r? @workingjubilee

@KodrAus
Copy link
Contributor

KodrAus commented Nov 15, 2020

Ah great! It works now 🙂

@workingjubilee
Copy link
Member

Oh cool! We should be good then!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2020

📌 Commit 72b83af has been approved by workingjubilee

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 18, 2020
Updated the list of white-listed target features for x86

This PR both adds in-source documentation on what to look out for when adding a new (X86) feature set and [adds all that are detectable at run-time in Rust stable as of 1.27.0](https://github.com/rust-lang/stdarch/blob/master/crates/std_detect/src/detect/arch/x86.rs).

This should only enable the use of the corresponding LLVM intrinsics.
Actual intrinsics need to be added separately in rust-lang/stdarch.

It also re-orders the run-time-detect test statements to be more consistent
with the actual list of intrinsics whitelisted and removes underscores not present
in the actual names (which might be mistaken as being part of the name)

The reference for LLVM's feature names used is [this file](https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/X86TargetParser.def).

This PR was motivated as the compiler end's part for allowing rust-lang#67329 to be adressed over on rust-lang/stdarch
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#78361 (Updated the list of white-listed target features for x86)
 - rust-lang#78785 (linux: try to use libc getrandom to allow interposition)
 - rust-lang#78999 (stability: More precise location for deprecation lint on macros)
 - rust-lang#79039 (Tighten the bounds on atomic Ordering in std::sys::unix::weak::Weak)
 - rust-lang#79079 (Turn top-level comments into module docs in MIR visitor)
 - rust-lang#79114 (add trailing_zeros and leading_zeros to non zero types)
 - rust-lang#79131 (Enable AVX512 *epi64 variants by updating stdarch)
 - rust-lang#79133 (bootstrap: use the same version number for rustc and cargo)
 - rust-lang#79145 (Fix handling of panic calls)
 - rust-lang#79151 (Fix typo in `std::io::Write` docs)
 - rust-lang#79158 (type is too big -> values of the type are too big)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7e9029 into rust-lang:master Nov 18, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 18, 2020
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 16, 2022
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? `@workingjubilee`

cc `@Amanieu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ``@workingjubilee``

cc ``@Amanieu``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ```@workingjubilee```

cc ```@Amanieu```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ```@workingjubilee```

cc ```@Amanieu```
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ```@workingjubilee```

cc ```@Amanieu```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants