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

Should -Ctarget-feature go straight to LLVM? #49653

Open
hanna-kruppe opened this issue Apr 4, 2018 · 17 comments
Open

Should -Ctarget-feature go straight to LLVM? #49653

hanna-kruppe opened this issue Apr 4, 2018 · 17 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stability Area: `#[stable]`, `#[unstable]` etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

The --print target-features and -C target-feature options list/accept all target features LLVM knows, under the names LLVM gives them (see discussion in #49428). This is in contrast to #[target_feature] and cfg(target_feature), which accept only explicitly whitelisted features, and in some cases change the LLVM names (e.g., bmi1 instead of bmi). This is inconsistent, and also makes the command line interface less stable than it could be.

As @gnzlbg noted in #49428, this difference has existed for a while. However, in effect the command line options don't have a real stability guarantee: rustcs not built with our LLVM fork don't currently recognize any target features, and the LLVM names can change under our feet (this was part of the rationale for having a whitelist in rustc). Note that -C flags "without stability guarantee" are not without precedent, e.g., consider -C passes (which also depends on LLVM internals).

So I believe we're within our rights to change this. Especially now that the whitelist is much more complete. And it has real advantages: consistency between command line and attributes/cfgs, more stability for the command line switch, and making it work on rustcs with system LLVMs, thanks to @cuviper's work in #49428.

cc @japaric are you aware of uses of this option that aren't covered by the current whitelists?

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 4, 2018

So in RFC2045 which #44839 tracks the consensus was basically the following:

  • adding a new CLI option: -C enable-features=sse,avx that is consistent with #[target_feature(enable = "sse")] and that errors on unknown and un-white-listed features.
  • deprecating -C target-feature=+sse,+avx on stable Rust and making it an error to use this on stable after one release cycle or so.
  • keeping -C target-feature=+sse,+avx on nightly Rust as the correct way to bypass the whitelist and talk directly to LLVM. Sometimes you really need to do this, for example, when targeting something non-standard, and we shouldn't really make the life of those doing this unnecessarily more difficult. Also, playing with non-white-listed target features is something that we might want to encourage on nightly. It's a good way of checking whether an LLVM feature does what you think it does without having to modify rustc to do so.

So I believe we're within our rights to change this.

So in the RFC everybody agreed that while this is the case users don't care: this would be a breaking change in the API of rustc, RUSTFLAGS , and some scripts that rely on this would need to be updated. Everybody agreed that the "defacto stabilization" of -C target-feature was unfortunate and a breaking change should be, if possible, avoided. If a breaking change was necessary, it should definitely come with a deprecation period.


So that was all that I can recall. Personally, I still think that the plan sketched in the RFC isn't that bad. I dislike having two ways to enable target features on nightly, and I dislike the naming a bit, but at least on nightly being able to bypass rustc's whitelist still feels necessary to me. Maybe -C enable-target-feature=sse,avx for the white-listed ones and -C llvm-target-feature=+sse,+avx would be clearer names, but renaming -C target-feature=+sse,+avx to -C llvm-target-feature=+sse,+avx might be felt by some as unnecessary churn. I would be slightly in favor of the re-name to make things clearer, but don't have a strong opinion about this.

If we are going to rename things, we should probably do so before stabilizing std::arch so that the number of people using -C target-feature on stable doesn't suddenly explode just before we break it.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 4, 2018

cc @alexcrichton

@hanna-kruppe
Copy link
Contributor Author

How is removing the flag entirely from stable compilers less of a compatibility break than removing some of the less commonly used values that can be passed to it today?

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Apr 4, 2018

I have more sympathy for killing the +feat/-feat syntax in favor of enable/disable flags consistent with the attribute syntax, I wasn't aware of that argument. I'd also be happy with killing -Ctarget-feature entirely if we can get away with it — it seems much more disruptive than applying whitelisting to the existing flag. I am not aware of any use case for the non-whitelisted features, and if it's a sensible use case we want to support, we can always add the feature to the whitelist.

Edit: to be clear, I believe if we can get away with making -C target-feature nightly-only, we can just as well remove it entirely.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 4, 2018

How is removing the flag entirely from stable compilers less of a compatibility break than removing some of the less commonly used values that can be passed to it today?

It isn't and I think that is an alternative worth considering. We could keep -C target-feature=+sse,-avx as is but just warn/error on non-white-listed features and on disabling features on stable/nightly.

Besides, I am not aware of any use case for the non-whitelisted features, and if it's a sensible use case we want to support, we can always add the feature to the whitelist.

Sometimes when targeting arm, mips, and powerpc, where not that many features are whitelisted, it was useful to be able to play with all the features that LLVM supports. For enabling simd on these platforms one feature is rarely enough (one might need to enable floating-point support + simd, for example).

Having said this, with time all these features will be whitelisted. Also, modifying the whitelist is pretty trivial. I would prefer if we would support a way of passing LLVM features directly on nightly for experimentation purposes, but I don't have strong feelings about this. @japaric does xargo rely on being able to pass features directly to LLVM ?

@cuviper
Copy link
Member

cuviper commented Apr 4, 2018

However, in effect the command line options don't have a real stability guarantee: rustcs not built with our LLVM fork don't currently recognize any target features,

Is that really true? I think the change I made to LLVMRustHasFeature only really affects #[cfg(target_feature = "foo")], but AFAIK -Ctarget-feature=+foo has always passed through to LLVM regardless.

and the LLVM names can change under our feet

On that note, we also support -Cllvm-args=val, which I have occasionally used for debugging codegen issues. I think this level of control really is valuable, as long as it's made clear where the definitions are coming from. In the case of -Ctarget-feature, perhaps we can just document that these are flags to be interpreted by the backend? (which someday might not even be LLVM at all!)

@hanna-kruppe
Copy link
Contributor Author

Is that really true? I think the change I made to LLVMRustHasFeature only really affects #[cfg(target_feature = "foo")], but AFAIK -Ctarget-feature=+foo has always passed through to LLVM regardless.

Oh duh you're right, the string is passed directly to LLVM. Its effect on cfgs and the like varies by linked LLVM version though, so there's still a problem (not to mention the other source of instability, LLVM changing its definition of the feature).

On that note, we also support -Cllvm-args=val, which I have occasionally used for debugging codegen issues. I think this level of control really is valuable, as long as it's made clear where the definitions are coming from. In the case of -Ctarget-feature, perhaps we can just document that these are flags to be interpreted by the backend? (which someday might not even be LLVM at all!)

If we keep having such an option, I'd prefer to call it -Z llvm-target-feature and either repurpose -C target-feature for the whitelisted and renamed features, or remove it entirely in favor of enable/disable features. It's a footgun to use such a prominent name for a debugging option. (By the same argument, I really dislike -C passes.)

@alexcrichton
Copy link
Member

I feel the same as @gnzlbg I think in this regard. I think we should add -C enable-target-feature which uses our own whitelist and effectively deprecated -C target-feature. That could either be renamed to -C llvm-target-feature or -Z, and if no one complains about it then it should be relatively ok to do in a point release

@cuviper
Copy link
Member

cuviper commented Apr 4, 2018

Does -C target-cpu have the same concern? I think that is passed directly to LLVM too.

@alexcrichton
Copy link
Member

@cuviper AFAIK yeah, we pass that straight to LLVM and don't really have any practical stability guarantees there

@pietroalbini pietroalbini added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2018
@hanna-kruppe
Copy link
Contributor Author

@cuviper Yes, but since it doesn't have a whitelisted counterpart (attributes or whatever) like target features have, there's no inconsistency, just yet another option that appears stable but actually isn't, so I wouldn't count it under this issue specifically.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 5, 2018

On one hand, it would be weird to require nightly Rust to be able to specify the target cpu. On the other, I don't see how we can guarantee that we will always support some cpu for any backend whatsoever.

@steveklabnik
Copy link
Member

Triage: not aware of any changes here, the problem is still the same.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 22, 2019

@steveklabnik we don't have a label for accidental stabilizations, which makes them hard to find (there are a couple of accidental stabilizations issues opened). It might make sense to create a label for it and start using it. cc @jonas-schievink

@jonas-schievink jonas-schievink added the A-stability Area: `#[stable]`, `#[unstable]` etc. label Sep 22, 2019
@jonas-schievink
Copy link
Contributor

There's A-stability, which kinda fits

@mattico
Copy link
Contributor

mattico commented Apr 7, 2021

One thing which would help would be to separate --print target-features into those supported by rustc and those which are just passed to LLVM. It could also explain the context around why and warn that it may change in the future.

I'll see if I can implement that.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 8, 2021

cc #44815 ("rustc silently ignores invalid -C target-feature names")

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 9, 2021
…ments, r=petrochenkov

Categorize and explain target features support

There are 3 different uses of the `-C target-feature` args passed to rustc:
1. All of the features are passed to LLVM, which uses them to configure code-generation. This is sort-of stabilized since 1.0 though LLVM does change/add/remove target features regularly.
2. Target features which are in [the compiler's allowlist](https://github.com/rust-lang/rust/blob/69e1d22ddbc67b25141a735a22a8895a678b32ca/compiler/rustc_codegen_ssa/src/target_features.rs#L12-L34) can be used in `cfg!(target_feature)` etc. These may have different names than in LLVM and are renamed before passing them to LLVM.
3. Target features which are in the allowlist and which are stabilized or feature-gate-enabled can be used in `#[target_feature]`.

It can be confusing that `rustc --print target-features` just prints out the LLVM features without separating out the rustc features or even mentioning that the dichotomy exists.

This improves the situation by separating out the rustc and LLVM target features and adding a brief explanation about the difference.

Abbreviated Example Output:
```
$ rustc --print target-features
Features supported by rustc for this target:
    adx                         - Support ADX instructions.
    aes                         - Enable AES instructions.
...
    xsaves                      - Support xsaves instructions.
    crt-static                  - Enables libraries with C Run-time Libraries(CRT) to be statically linked.

Code-generation features supported by LLVM for this target:
    16bit-mode                  - 16-bit mode (i8086).
    32bit-mode                  - 32-bit mode (80386).
...
    x87                         - Enable X87 float instructions.
    xop                         - Enable XOP instructions.

Use +feature to enable a feature, or -feature to disable it.
For example, rustc -C target-cpu=mycpu -C target-feature=+feature1,-feature2

Code-generation features cannot be used in cfg or #[target_feature],
and may be renamed or removed in a future version of LLVM or rustc.

```

Motivated by rust-lang#83975.
CC rust-lang#49653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-stability Area: `#[stable]`, `#[unstable]` etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants