Tracking issue for `cfg_target_feature`
Closed this issue · 55 comments
The only known issue with this I'm aware of is that it doesn't doesn't use the same mechanism in LLVM to query whether a feature is activated or not. We already maintain our own list of what features are allowed here (e.g. we're not just exposing what LLVM accepts) and then we currently map that down to an LLVM feature name. When testing whether the LLVM feature is activated, however, we should robustly query LLVM via its own internal mechanisms rather than explicitly checking ourselves.
I opened a separate issue #30462 for the missing cfg_target_feature for bit manipulation instruction sets.
cfg target_feature -- for github issue searchability as well
also a new forum link -- https://internals.rust-lang.org/t/comprehensive-list-of-desired-cfg-target-feature-s/3201
So this is a list of the current target features supported by llc -mattr=help:
16bit-mode - 16-bit mode (i8086).
32bit-mode - 32-bit mode (80386).
3dnow - Enable 3DNow! instructions.
3dnowa - Enable 3DNow! Athlon instructions.
64bit - Support 64-bit instructions.
64bit-mode - 64-bit mode (x86_64).
adx - Support ADX instructions.
aes - Enable AES instructions.
atom - Intel Atom processors.
avx - Enable AVX instructions.
avx2 - Enable AVX2 instructions.
avx512bw - Enable AVX-512 Byte and Word Instructions.
avx512cd - Enable AVX-512 Conflict Detection Instructions.
avx512dq - Enable AVX-512 Doubleword and Quadword Instructions.
avx512er - Enable AVX-512 Exponential and Reciprocal Instructions.
avx512f - Enable AVX-512 instructions.
avx512ifma - Enable AVX-512 Integer Fused Multiple-Add.
avx512pf - Enable AVX-512 PreFetch Instructions.
avx512vbmi - Enable AVX-512 Vector Bit Manipulation Instructions.
avx512vl - Enable AVX-512 Vector Length eXtensions.
bmi - Support BMI instructions.
bmi2 - Support BMI2 instructions.
call-reg-indirect - Call register indirect.
clflushopt - Flush A Cache Line Optimized.
clwb - Cache Line Write Back.
cmov - Enable conditional move instructions.
cx16 - 64-bit with cmpxchg16b.
f16c - Support 16-bit floating point conversion instructions.
fast-partial-ymm-write - Partial writes to YMM registers are fast.
fma - Enable three-operand fused multiple-add.
fma4 - Enable four-operand fused multiple-add.
fsgsbase - Support FS/GS Base instructions.
fxsr - Support fxsave/fxrestore instructions.
hle - Support HLE.
idivl-to-divb - Use 8-bit divide for positive values less than 256.
idivq-to-divw - Use 16-bit divide for positive values less than 65536.
invpcid - Invalidate Process-Context Identifier.
lea-sp - Use LEA for adjusting the stack pointer.
lea-uses-ag - LEA instruction needs inputs at AG stage.
lzcnt - Support LZCNT instruction.
mmx - Enable MMX instructions.
movbe - Support MOVBE instruction.
mpx - Support MPX instructions.
mwaitx - Enable MONITORX/MWAITX timer functionality.
pad-short-functions - Pad short functions.
pclmul - Enable packed carry-less multiplication instructions.
pcommit - Enable Persistent Commit.
pku - Enable protection keys.
popcnt - Support POPCNT instruction.
prefetchwt1 - Prefetch with Intent to Write and T1 Hint.
prfchw - Support PRFCHW instructions.
rdrnd - Support RDRAND instruction.
rdseed - Support RDSEED instruction.
rtm - Support RTM instructions.
sahf - Support LAHF and SAHF instructions.
sgx - Enable Software Guard Extensions.
sha - Enable SHA instructions.
slm - Intel Silvermont processors.
slow-bt-mem - Bit testing of memory is slow.
slow-incdec - INC and DEC instructions are slower than ADD and SUB.
slow-lea - LEA instruction with certain arguments is slow.
slow-shld - SHLD instruction is slow.
slow-unaligned-mem-16 - Slow unaligned 16-byte memory access.
slow-unaligned-mem-32 - Slow unaligned 32-byte memory access.
smap - Supervisor Mode Access Protection.
soft-float - Use software floating point features..
sse - Enable SSE instructions.
sse-unaligned-mem - Allow unaligned memory operands with SSE instructions.
sse2 - Enable SSE2 instructions.
sse3 - Enable SSE3 instructions.
sse4.1 - Enable SSE 4.1 instructions.
sse4.2 - Enable SSE 4.2 instructions.
sse4a - Support SSE 4a instructions.
ssse3 - Enable SSSE3 instructions.
tbm - Enable TBM instructions.
vmfunc - VM Functions.
x87 - Enable X87 float instructions.
xop - Enable XOP instructions.
xsave - Support xsave instructions.
xsavec - Support xsavec instructions.
xsaveopt - Support xsaveopt instructions.
xsaves - Support xsaves instructions.Would a patch that implements macros for all of these need to go through the RFC process?
cc @BurntSushi @eddyb @nikomatsakis -- I believe all of you were recently involved in discussion around SIMD stabilization.
This would be useful to use in matrixmultiply, ndarray even before simd stabilization. I get better results if I can pick the particular stable rust-implemented loop that generates best code given the particular available vector types / available target features.
For people interested in helping with this:
There is a allowlist of features that are exposed by this mechanism, which is very limited. See https://github.com/rust-lang/rust/blob/master/src/librustc_driver/target_features.rs#L23-L28, in particular the variables ARM_WHITELIST and X86_WHITELIST.
I found #34412 which seems to be a good example of a PR that expands the set of features that can be used with #[cfg(target_feature=)]. It looks straightforward to create more PRs that expose more features, and I encourage people to do so.
When testing whether the LLVM feature is activated, however, we should robustly query LLVM via its own internal mechanisms rather than explicitly checking ourselves.
Is this blocking stabilization of this feature? And, if so, is this the only thing blocking stabilization, or are there other things?
When testing whether the LLVM feature is activated, however, we should robustly query LLVM via its own internal mechanisms rather than explicitly checking ourselves.
Is this blocking stabilization of this feature? And, if so, is this the only thing blocking stabilization, or are there other things?
To answer my own question: IMO, the above concern doesn't need to block this feature from being stabilized. In my case, I don't care whether LLVM has the CPU features enabled in its code generation because I'm not using LLVM-based code gen for my code that uses SIMD and stuff.
The above concern does need to block the stabilization of intrinsics and other things exposed from libcore/libstd that depend on #[cfg(target-feature)], but that's separate from exposing #[cfg(target-feature)] itself, and that should happen in the tracking issues for those features, not this one.
So, IMO, this seems like it this is good to go.
One clarification might be in order: When code uses this feature, can it assume that every feature name will be globally unique, or should it assume that feature names might be reused across different architectures. For example, would a feature "+crypto" imply AAarch32 or AAarch64, or is it possible that, say, a MIPS target could have a "+crypto" feature? In some instances this would affect whether one writes this:
#[cfg(target_feature="crypto")]or this:
#[cfg(all(target_feature="crypto", any(target="aarch64", target="aarch32")))]@briansmith AFAIK this issue is not block on anything. The concern you linked to about querying LLVM directly was actually resolved back in April!
Also AFAIK we haven't discussed issues of namespaces or implication of features across architectures.
@briansmith AFAIK this issue is not block on anything. The concern you linked to about querying LLVM directly was actually resolved back in April!
Awesome!
Also AFAIK we haven't discussed issues of namespaces or implication of features across architectures.
The RFC says "This RFC proposes a target_feature cfg, that would be set to the features of the architecture that are known to be supported by the exact target e.g." To me, that implies that some feature names may overlap between architectures. This means that one probably should use target_arch or similar in addition to target_feature, although the example in the RFC doesn't do that.
I also noticed that the documentation for each target feature is missing. In particular, there doesn't seem to be any documentation describing what each value of target_feature means, nor is there documentation indicating the implications. For example, the RFC says that +sse42 implies the enabling of earlier versions of SSE, and these implied enablings should be documented.
For example, in the case of Aarch32/Aarch64 crypto features, ARM has defined a "crypto" feature that enables AES, SHA-1, and SHA-2 instructions. However, the CPU actually reports whether each feature is enabled/disabled separately, so in theory there could be individual sub-features of "crypto" in the future. Also, in clang "+crypto" enables floating point and NEON, according to its documentation.
If it is expected that the Rust target features will stay in sync with LLVM's, then we could just have documentation for target_feature that points to the external LLVM documentation. That's what I'd recommend for now.
Note in particular the GCC documentation says "feature crypto implies simd, which implies fp. Conversely, nofp implies nosimd, which implies nocrypto." (emphasis mine). Does this mean that in Rust -fp should imply -simd and -simd should imply -crypto? What about things like `+crypto -simd"?
@briansmith yeah makes sense to me that we'd want to make sure we document all supported target features in one way or another, including hierarchies and implications amongst them.
I do believe we support -C target-feature=-foo, although I don't know if it also has those sorts of implication changes (we just rely on LLVM for now). Would be good to find out!
What's the relation to cargo's CARGO_CFG_TARGET_FEATURE that it gives to build scripts? This env var is possible to use from Rust 1.14 (from the cargo version shipped with it) and the project can set its own cfg vars depending on which features are enabled.
Cargo gained a feature recently to ferry along everything from rustc --print cfg over to build scripts, which is where CARGO_CFG_TARGET_FEATURE comes from. The --print cfg option is stable as well and is basically orthogonal to this feature itself.
The #![feature(cfg_target_feature)] in this issue is specifically related to #[cfg(target_feature = "...")] annotations. Note, though, that many of those target features (e.g. sse, etc), are only set on nightly. This means that --print cfg on stable doesn't print those features, but it does on nightly. So this issue is also pseudo sort-of for all those names as well, and allowing them to be stable.
In other words, @briansmith's points about documentation, relationships, names, what they actually mean, etc, I believe are also all relevant for this stabilization issue.
Ok that's important information. I don't like that stable prints something different, but it's important to know.
Very late clarify (March 2017): Normally, on nightly we opt in to nightly features (such as printing more cfg's), so it's surprising when it is a silently enabled stable/nightly difference.
I'd like to hopefully make progress on this issue as the real issue I'd like to make progress on, #37406, I have now realized is blocked on this report.
As of today we have solved my original concern by querying LLVM directly about this information instead of attempting to infer it ourselves. However @briansmith's concern remains about namespacing across targets and such.
I would also like to show that practically today all our features match LLVM exactly (we don't change the names). This is intentional for these features but we are not tying ourselves to LLVM's naming, we reserve the right to change the name and add features (such as crt-static). I will also point out, though, that adding more features is very easy to do and is highly likely to skip stabilization processes and such.
As a result I would like to propose the following:
- Stabilize
-C target-feature - Stabilize
#[cfg(target_feature)] - Stabilize
--print cfgprinting the target features - Do not stabilize any target feature names
That is, I'd like to stabilize this mechanism for controlling features to targets, and then leave stabilization of the names here to the actual relevant subsystems. For example I would expect SIMD stabilization to be coupled with the SIMD attributes, and crt-static should have its own bikeshed for the name.
This does force our hand I think on the "global namespace" argument where it would be mega confusing for x86_64 Linux to have a foo feature that's different from the arm Windows foo feature. This may mean that some features need to be renamed appropriately to prefix the relevant information. Other features, though, like crt-static apply to all targets equally.
Along those lines, I will...
@rfcbot fcp merge
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:
- @alexcrichton
- @aturon
- @brson
- @carols10cents
- @eddyb
- @japaric
- @michaelwoerister
- @nikomatsakis
- @nrc
- @pnkfelix
- @vadimcn
- @withoutboats
- @wycats
No concerns currently listed.
Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
@rfcbot reviewed
What should rustc do when it encounters an unknown target feature?
I think there should be consensus on that before stabilizing this feature.
I think it should "return false" that is, silently (by default) say that the feature is disabled.
We should also provide an opt-in warning to diagnose unknown target features.
Ideally code that uses target feature will compile on stable, but only those features that are stabilized will be "potentially" enabled by a stable compiler. Ignoring the rest seems to me the sensible thing to do. Those wanting to benefit from the other features will do so in nightly.
I would also have expected a small RFC for this, with the proposed semantics, how the stabilization of the features should work, the behavior of target feature with unknown features (in stable, where one probably wants to ignore at least the ones that are known to a nightly compiler), the behavior of unknown features in nightly (where they are probably an error), etc.
It would be cool if rustc could print all target features:
- that are available in LLVM,
- that are available for the current host target,
- that are available for some particular target.
@gnzlbg do you feel that the currently implemented behavior is incorrect? Currently the compiler only sets #[cfg] attributes for what it's defined (nothing on stabled, enabled ones on nightly) and from the command line it looks like unknown features are warnings (albeit warnings printed out by LLVM so they look bad, but warnings nonetheless).
I'd ideally like to not delay this even further, so if we can reach consesus on this issue itself that'd be best :)
do you feel that the currently implemented behavior is incorrect?
I feel that the correct behavior of this feature (in particular, on stable) is not documented anywhere nor has been discussed at all here.
Without knowing what the correct behavior should be, it is impossible to know whether the currently implemented behavior is incorrect.
I would expect that the discussion here would be summarized in a 1-2 page long RFC and PR'ed to the RFC repo so that anybody interested can chime it. I also expect consensus in <1week for something this small, which means you can have it merged in about 2 weeks for now.
I expect the following points addressed on the RFC (beyond motivation, how do we teach this, etc.):
- A summary of the discussion here, and on the SIMD post on the internals forum, in which target feature has been discussed extensively as a key element of future SIMD support.
- The behavior of rustc for the following four types of target feature flags on both nightly and stable: stabilized, known to rustc but unstabilized, unknown to rustc but known in LLVM, unknown to LLVM. In particular, whether it warns, hard errors, or just disables the feature by default on stable.
- I think this is a primer in which a feature that takes arguments is stabilized, but no stable arguments are available. One or two lines explaining why/how this is done would be neat.
For example, I would expect all nightly crates that only depend on cfg_target_feature to be able to remove the feature flag and compile on stable as is. If the target features they use are known to rustc/LLVM i would expect that those unstabilized target features are just turned off on stable without any warnings, and that an opt-in warning for this is available.
But this is only my opinion, and I would like to hear the opinions of other as well. It is however hard to discuss this when the behavior being proposed is not written down anywhere, and when this stabilization process has gotten close to zero exposure because its not going through the RFC repo, not announced in the internals forum, not announced on reddit, etc.
I'd ideally like to not delay this even further, so if we can reach consesus on this issue itself that'd be best :)
Not only you, a lot of people are interested on this happening ASAP. I still think it should go through a small and quick RFC process though.
In particular, you seem to know very clearly what the behavior that cfg_target_feature should be, and also, are confident enough that you want to stabilize it as it is currently implemented. So if it really is like this, just write it down, send a PR to the RFC repo, and it will go through the process smoothly and quickly.
@rfcbot reviewed
I agree this needs to be documented before considering it for stabilization. I certainly don't know how it works in any detail. The mechanism seems to predate the RFC process. It needs end-user documentation at the least.
[Meta]
This final-comment-period is not part of "This Week in Rust 174", probably because they just scan the RFC repo, and also, because this issue is not tagged here with an fcp label. I would like to be able to see everything in fcp in a single centralized place.
EDIT: I see that rust-lang issues in FCP are part of http://rusty-dash.com/fcp, I've pinged TWiR about this.
@gnzlbg indeed, issues (and PRs) on rust-lang/rust should absolutely be part of TWiR!
@gnzlbg I'm circling back to this issue, and wondering if you'd be open to helping write up an RFC (or internals post) along the lines you've suggested? The Rust team should be able to provide guidance/support as needed.
Sure I can write that, would it be possible for @alexcrichton to mentor it?
@gnzlbg should be! He's traveling right now but will likely respond soon. If you don't hear from him or he doesn't have time to take it on, reach out to me again and I'll try to hook you up with someone else.
@gnzlbg sure definitely! Feel free to reach out to me here, on email, or IRC if you need any help.
@alexcrichton and others: what is the level of urgency here? If we have a strong need to get this done, maybe we can have a synchronous meeting amongst stakeholders to hash out the details?
Empirically this seems not urgent. My motivation transitively stems from #37406 which I believe is important for a solid MSVC experience. It may be best to just completely decouple that from this.
@alexcrichton How difficult is such a decoupling? It seems like we're not so far from being in a position to stabilize target_feature, if we can flesh out the spec a bit (@gnzlbg, still planning to do that?)
I'm unsure of how much work it would entail or if it's even feasible. It's mostly just an opportunity to stabilize that sooner.
So I managed to come up with something: https://internals.rust-lang.org/t/pre-rfc-stabilization-of-target-feature/5176 (its a bit rough, but I hope it can kickstart the discussion towards an RFC, haven't had much free time lately).
It sketches a plan for stabilization, and mentions what should be implemented before that happens. In a nutshell we just need two white-lists (one for stable and one for unstable target features), a way to error/warn on stable rust when unknown/unstable target features are used, and a way for users to silence those warnings and disable those features on stable.
🔔 This is now entering its final comment period, as per the review above. 🔔
The final comment period is now complete.
#44367 is about repr(simd) and tangentially about #[target_feature]. I don't think it has anything to do with cfg_target_feature itself.
It worries me a bit thatcfg_target_feature has one open bug (#42515) that does not have a clear path towards a solution because the current RFC specifies that this must work.
OTOH fixing this bug is backward compatible, and if it cannot be fixed, then cfg_target_feature is ready as is and the RFC will be amended. So I don't see any major reasons why we could not stabilize cfg_target_feature already.
Personally, I'd rather wait till the whole RFC (rust-lang/rfcs#2045) is merged and implemented . IIUC correctly the main motivation to stabilize cfg_target_feature ASAP has disappeared with @alexcrichton fix of #37406 .
The FCP here appears to have completed back in August. Is this just waiting for a stabilization PR?
The implementation of this feature started implemented this week (#47223). So we have <1 week experience with using the initial implementation of it. A PR to update stdsimd to use it was submitted yesterday (rust-lang/stdarch#283).
Also, there is one bug open that the accepted RFC required to be fixed before stabilization:
And we should probably gain some experience with the feature once that bug is fixed. Also, we probably want to fix #44367 before stabilizing this as well, and gain some experience with the solution.
By needing to gain some experience I don't mean waiting one year, but rather, updating stdsimd and using the feature in its examples, checking that everything works as expected.
Also, I don't completely understand the FCP/RFC process in detail, but my mental model is that it looks like this:
- design -> RFC
- RFC -> implementation
- implementation -> nightly feature
- nightly feature -> experience
- experience -> tweak design
- ...
- design + implementation + experience -> stabilize
We are still at the RFC -> implementation phase here (step 2 out of N), so having a FCP about this feature was probably an oversight. Would it be possible to cancel the FCP, and re-open it once the feature is completely implemented?
Yeah as pointed out by @gnzlbg the original motivation for stabilizing this more quickly evaporated which caused this to sit for awhile. Work has started on the implementation and the next outstanding piece I believe is gating each feature name behind a separate feature gate.
This is now pretty tightly coupled with simd stabilization (without many other motivational factors) so I wouldn't expect this to stabilize ahead or long after SIMD (but rather at the same time). SIMD's still farther out, though.
Counter-intuitively, specifying -C target-feature=rdrnd on the rustc command line and using #[cfg(target_feature = "rdrnd")] does not work - the #[cfg()] behaves as if -C target-feature=rdrnd wasn't set. However, setting a target feature within a target-specification.json file does work.
Also, compiling against a native CPU with this feature available also doesn't cause #[cfg(target_feature = "rndrd")] to be true.
Is this intentional?
@raphaelcohn I believe you need -C target-feature=+rdrnd.
@raphaelcohn I believe you need -C target-feature=+rdrnd.
If -C target-feature=rdrnd doesn't produce an error, it should. @raphaelcohn mind filling a rust-lang/rust issue ?
I'll double-check I transcribed that correctly. However, if it is supposed to +rdrnd, then it'd be nice if the same syntax was also used in #[target_feature(enable)], too (it seems it was before enable came along).
Right, target-feature=rdrnd doesn't error, target-feature=+rdrnd doesn't error, but does match for #[cfg(target_feature = "rdrnd")]. So a bug, then?
@raphaelcohn the RFC says
There are currently two ways of passing target feature information to rustc's code generation backend on stable Rust.
-C --target-feature=+/-backend_target_feature_name: where+/-add/remove features from the default feature set of the platform for the whole crate.
-C --target-cpu=backend_cpu_name, which changes the default feature set of the crate to be that of all features enabled for backend_cpu_name.These two options are available on stable Rust and have been defacto stabilized. Their semantics are LLVM specific and depend on what LLVM actually does with the features.
This RFC proposes to keep these options "as is", and add one new compiler option,
--enable-features="feature0,feature1,...", (the analogous --disable-features is discussed in the "Future Extensions" section) that supports only stabilized target features.
That is,
then it'd be nice if the same syntax was also used in #[target_feature(enable)], too (it seems it was before enable came along).
per the rfc, the analogous syntax is --enable-features="...", and might not be implemented yet.
I think that target-feature=rdrnd should error.
#48556 has entered FCP which I believe will implicitly stabilize this attribute