Add -Zllvm-target-feature to pass features directly to LLVM - #159545
Add -Zllvm-target-feature to pass features directly to LLVM#159545fo40225 wants to merge 1 commit into
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @nnethercote (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
The warning-based passthrough is unsound to rely on
I do not understand what you mean by "unsound" here, @fo40225.
It is undesirable to rely on.
Passing target features at all, however, is simply prone to being unsound, because LLVM can have arbitrary semantics for "target features" that do not accord with the model adopted for Rust target features. It is not relying on the passthrough that is unsound.
Anyways, please rewrite your entire PR description for conciseness. Or, if you had something else "write" it, then actually do so for the first time. Either way, I do not need to be regaled with the minutiae of every test if I can read the code. That entire PR description will become a commit in our repo, so please account for people staring at it in their terminal window after tapping out git status.
Also, please split out the addition of the future compatibility warning. This does not need to, and should not, all happen in one PR. Make this add -Zllvm-target-feature only.
| features in the form `+feat` or `-feat`, for example: | ||
|
|
||
| ```sh | ||
| rustc -Zllvm-target-feature=+prefer-256-bit main.rs |
There was a problem hiding this comment.
Example should exemplify the multi-feature case (either in the same invocation or in another).
| ``` | ||
|
|
||
| Each feature string is forwarded verbatim to LLVM. This allows using LLVM target features | ||
| that are not (or not yet) part of Rust's target feature system, for example to match the |
There was a problem hiding this comment.
This can be less ambiguous: some things LLVM calls "target features" should never be part of ours, because they e.g. are incoherent with target_feature(enable), which allows adding them on a per-function basis, and only make sense as whole-program modifiers.
|
|
Tracking issue: 157753
b9d5330 to
3f5cc28
Compare
|
I've found it difficult to implement this functionality reasonably. My thoughts are as follows: Scenario 1: This completely breaks the functionality of Scenario 2: Design A: Maintain a mapping table between Rust Design B: Directly change Unless there are other new design guidelines, I will abandon this PR. |
|
I only just saw this, after already implementing the same thing myself in #160457. Please post in the tracking issue when a PR is opened to avoid duplicate work. I am not sure which problem you are talking about in your last comment.
What's
Rust is the compiler, so what are you talking about? |
|
☔ The latest upstream changes (presumably #160517) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
-Ctarget-featurepasses unrecognized features through to LLVM with a warning.Users who need LLVM-only features (e.g. the Linux kernel) must either tolerate the warning or resort to JSON target specs.
This flag provides a dedicated mechanism, registered as a target modifier.
Tracking issue: #157753