Skip to content

Enable target_feature on any LLVM 6+ #49428

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Mar 27, 2018

In LLVMRustHasFeature(), rather than using MCInfo->getFeatureTable()
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

/// Check whether the subtarget features are enabled/disabled as per
/// the provided string, ignoring all other features.
bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have target_feature.

r? @alexcrichton

In `LLVMRustHasFeature()`, rather than using `MCInfo->getFeatureTable()`
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

    /// Check whether the subtarget features are enabled/disabled as per
    /// the provided string, ignoring all other features.
    bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have `target_feature`.
@cuviper cuviper requested a review from alexcrichton March 27, 2018 20:01
@cuviper cuviper added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2018
@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

(The bot seems to be asleep, so I've manually set the requested reviewer...)

@hanna-kruppe
Copy link
Contributor

Does this mean we can drop rust-lang/llvm@a8a60a1?

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

@rkruppe those tables are still used for rustc --print target-cpus and --print target-features, but I'm less worried about those since they're just informative.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

(An alternative for that same info is llc -mcpu=help.)

@hanna-kruppe
Copy link
Contributor

Surely --print target-features should use rustc's whitelist (and differing names)? Good point about --print target-cpus though.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2018

I think --print target-features is meant to help feed -C target-feature, which operates at the LLVM level. So I'm not sure the whitelist and differing names are applicable.

@hanna-kruppe
Copy link
Contributor

Ugh, that's news to me. IMO the -C option should match the attributes, including whitelisting and renaming. But that's beyond the scope of this PR.

@alexcrichton
Copy link
Member

@bors: r+

Awesome, thanks so much @cuviper!

@bors
Copy link
Collaborator

bors commented Mar 27, 2018

📌 Commit a93a4d2 has been approved by alexcrichton

@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 Mar 27, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Mar 28, 2018
…xcrichton

Enable target_feature on any LLVM 6+

In `LLVMRustHasFeature()`, rather than using `MCInfo->getFeatureTable()`
that is specific to Rust's LLVM fork, we can use this in LLVM 6:

    /// Check whether the subtarget features are enabled/disabled as per
    /// the provided string, ignoring all other features.
    bool checkFeatures(StringRef FS) const;

Now rustc using external LLVM can also have `target_feature`.

r? @alexcrichton
bors added a commit that referenced this pull request Mar 28, 2018
Rollup of 12 pull requests

- Successful merges: #49243, #49329, #49364, #49400, #49405, #49427, #49428, #49429, #49439, #49442, #49444, #49452
- Failed merges:
@bors bors merged commit a93a4d2 into rust-lang:master Mar 29, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 4, 2018

@rkruppe the problem is that -C target-feature has been available on stable since forever and has always mapped to LLVM directly, so changing that was considered a backwards incompatible change. The target feature RFC added -C feature-enable=... (or something like that) to map to Rust target features but it is not implemented yet.

@hanna-kruppe
Copy link
Contributor

@gnzlbg let's hash this out in #49653

# for free to join this conversation on GitHub. Already have an account? # 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.

5 participants