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

Support detecting all llc's x86 target features #34397

Closed
wants to merge 3 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jun 21, 2016

This commit adds support for detecting all of llc's target features for the x86/x86_64 architectures.

Closes #30462.
Helps with #29717 and #34382.

This commit adds support for detecting all of llc's target features for the x86/x86_64 architectures.

Closes rust-lang#30462.
Helps with rust-lang#29717 and rust-lang#34382.
@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 @nikomatsakis (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.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 21, 2016

pinging also: @huonw @alexcrichton

BTW: This PR doesn't solve the root of the issue (we should be getting the features that are available from LLVM directly instead of maintaining our own list).

@ranma42
Copy link
Contributor

ranma42 commented Jun 21, 2016

The original implementation of feature detection provided the complete list of LLVM features in Rust, but it was modified to only expose those that are explicitly whitelisted as per #31709 (comment)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 21, 2016

@ranma42

The original implementation of feature detection provided the complete list of LLVM features in Rust, but it was modified to only expose those that are explicitly whitelisted as per #31709 (comment).

How did you know which features to whitelist and which to remove?

@ranma42
Copy link
Contributor

ranma42 commented Jun 21, 2016

@gnzlbg before #31709 the available features were computed on a best-effort basis from the flags passed to the compiler; in #31709 I kept the pre-existing list of features from here: https://github.com/rust-lang/rust/pull/31709/files#diff-787403961a3c37598ae191c2d68be445L35

I think @huonw determined the list of features when as part of his SIMD work.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 21, 2016

So is @huonw the person responsible for white listing new features?

Or what is the procedure for white listing new features?

I just need a couple of feature flags (bmi, bmi2, abm, tbm, sse4a) but the only response I got in the forum is that if we want to not deal with this problem every time somebody needs to do something that requires a feature flag that is not exposed, the best thing would probably be to just expose all the ones that are available.

@alexcrichton
Copy link
Member

Yeah as pointed out by @ranma42 these features were historically hand-selected and parsed in an ad-hoc fashion (primarily for SIMD support). In #31709 it was fixed to compute these features directly from LLVM to have a more robust implementation (e.g. working with -C target-cpu=native), but it was also decided to not just blanket expose everything from LLVM. This would be a stability hazard because I don't think LLVM provides a backwards-compatibility guarantee with these names.

As a result, to add new features, we basically need to follow a checklist like:

  • Are we comfortable that the name itself? Do we feel that it's "rustic" or otherwise appropriate?
  • Are we confident LLVM will continue to have a feature for this in the future? If not we'd have to detect it at some point.

We've avoided "kitchen sink" style commits which just add a load of features that LLVM has to the compiler, as it's unfortunately easy for something we don't want to sneak in by accident.

Would it be possible to trim this list down to just what's needed?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 21, 2016

I just need "bmi, bmi2, abm, tbm, sse4a" so I can trim the list down to that. Those doing encryption might want aes but I would leave it out for now.

In #31709 it was fixed to compute these features directly from LLVM to have a more robust implementation (e.g. working with -C target-cpu=native)

Is there an easy way to do that? That would allow exposing avx512 as a feature flag if all the avx512 features are available (but I don't think anybody will be playing with avx512 from rust any time soon).

@alexcrichton
Copy link
Member

cc @rust-lang/tools or @rust-lang/lang, thoughts on the names for these features?

Note that these are not stable today (cfg(target_feature) is still unstable), so we've got some leeway no matter what. We just eventually need to consider the names when stabilizing target_feature

@brson
Copy link
Contributor

brson commented Jun 21, 2016

I don't have any better ideas. lgtm

@brson
Copy link
Contributor

brson commented Jun 21, 2016

Is there a command in rustc to list these values?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 21, 2016

FYI I will probably implement intrinsics for those instruction sets over the next couple of days (there are LLVM IR functions for most of the instructions in those instruction sets), so that I can write a crate that exposes the intrinsics as rust functions (in the same spirit of the SIMD crate, but without defining new types or traits, just the "bare" intrinsics).

We just eventually need to consider the names when stabilizing target_feature

I think we could stabilize target_feature (as a feature) without needing to actually mention any of the feature names by, for example, defining that feature names unknown to the rust implementation always return false (and maybe stating that a diagnostic, although encouraged, is left as "quality of implementation" issue). That way people could use the feature, but they should check the manual for the rustc versions to know which features are available. If we ever decide to deprecate or rename a feature crates could use the not,and,or to support multiple rustc versions transparently.

EDIT: In particular, I don't think we could ever stabilize the feature names. What if LLVM drops support for some architecture, or renames/coalesces them, adds/removes intrinsics to some features over time, ... I don't think it is worth going to something like "feature versioning" unless it turns out to really cause problems in practice.

@ranma42
Copy link
Contributor

ranma42 commented Jun 22, 2016

@brson rustc --print cfg prints the ones that are actually available, but I am afraid that currently there is no way to list all possible target_feature values.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 22, 2016

Superseeded by PR #34412 , which not only adds the targets but also implements the intrinsics in the compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants