Rust: Should -Ctarget-feature go straight to LLVM?

[hanna-kruppe]

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]

So in RFC2045 which https://github.com/rust-lang/rust/issues/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]

cc @alexcrichton

[hanna-kruppe]

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]

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]

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]

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]

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]

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]

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

[alexcrichton]

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

[hanna-kruppe]

@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]

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]

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

[gnzlbg]

@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]

There's A-stability, which kinda fits