Repository navigation
ACP: efficient runtime checking of multiple target features #585
Description
Activity
Am I missing something? Your example alternative code never checks for
bmi.ah right (I've been running a bunch of benchmarks with different configurations). Fixed now, thanks!
Prior relevant discussion: https://internals.rust-lang.org/t/better-codegen-for-cpu-feature-detection/22083
and eax, 2 xor ecx, ecx or rax, rcx setne al
even that code is inefficient, it could just be:
and eax, 2 shr eax, 1
or if it was immediately used for branching, just:
test eax, 2 jnz has_feature
This all makes sense, given that the cache is stored in an atomic, so the read value cannot be reused
I know in vtables we add some special LLVM things to tell it that re-reading will always give the same value, even if there's other stuff between.
Obviously we can't do that for the lazy-init part, but maybe in the normal case after that there'd be a way?
returns a bitmap of enabled features
Does it have to be a bitmap? Could we return some kind of x86-specific library type with
.avx2()and.bmi2()and such?(Or even let that type exist on all platforms, just trivially returns false for everything.)
Reacted by Peter JaszkowiakSure, it doesn't have to be literally a bitmap, there is a lot of freedom in exactly how to implement it.
One tricky thing is that the features are stored as 3 atomics, so depending on what features you ask for, one load might have all the bits you need, or you might need all 3 loads. So we don't want to repeat work when 2 features are stored in the same atomic value, but also don't want to pessimistically load all three values.
static CACHE: [Cache; 3] = [ Cache::uninitialized(), Cache::uninitialized(), Cache::uninitialized(), ]; struct Cache(AtomicUsize);
Also looking at this now, that static might benefit from
#[align]to make sure it gets its own cache line.Note that this has some interaction with the accepted RFC for adding splitting these macros into
coreandstdparts https://github.com/Amanieu/rfcs/blob/core-detect/text/0000-core_detect.md. (@sayantn expressed some interest in possibly picking up that part).In rust-lang/rfcs#3469 (comment) I argue that the whole system likely needs to be re-designed anyways, so this would be a good opportunity to provide a better API as part of the re-design.
What about
enum TargetFeature { Sse, Sse2, ... } pub fn are_features_detected<const N: size>(features: [TargetFeature; N]) -> bool;
I do think this is going to need to be redesigned in the future (e.g. for things like passing around a type-level proof that you have a given target feature), but in the short-term, I personally think it'd be reasonable to support passing multiple arguments to
is_x86_feature_detected!.We discussed this in today's meeting. @Amanieu observed that we may want support for "or" rather than "and". After some discussion, we landed on supporting boolean
&&and||syntax in the feature detection macros.is_x86_feature_detected!(("f1" && "f2") || "f3")We'd be happy to accept a PR implementing this syntax on nightly, for each of the feature detection macros. (The new syntax should be feature-gated, not insta-stable.)
Reacted by kennytmWhat’s an example of when one would want “or”? Every target feature check I’ve ever read or written has been “and” because it’s always about “can I soundly call these
core::archfunctions” and I don’t think rustc/LLVM even has a notion of “this function needs either this target feature or that target feature”. There are implications of the form “feat1 implies feat3, feat2 implies feat3” where feat1 and feat2 are independent or even mutually exclusive, but these implications should be resolved earlier so the user’s feature detection can just check the relevant feat3 directly.Reacted by kennytmBased on the meeting notes apparently RISC-V would get some use out of the "or" operation.
Reacted by Josh TriplettThis ACP has been accepted.
Please open a tracking issue and open a PR to rust-lang/rust to add it as an unstable feature. You can close this ACP once the tracking issue has been created.
Proposal
Problem statement
Currently, checking for whether two target features are enabled is inefficient. In zlib-rs we see a 3% slowdown in one test case from checking for an additional target feature.
Performing a runtime check for 2 target features requires roughly double the number of instructions versus checking for just one feature.
Motivating examples or use cases
In zlib-rs, we want to check for both the
avx2andbmi2features, but that check is slower than just checking foravx2.Looking at just the happy path (where the features are already cached and both are available):
https://godbolt.org/z/f935sP6dr
So checking for 2 features roughly doubles the number of instructions, and performs 2 (atomic) loads.
This all makes sense, given that the cache is stored in an atomic, so the read value cannot be reused, and the expansion looks like this:
Solution sketch
I'd like the macro to expand to something like this instead, where
__is_feature_detected()returns a bitmap of enabled features:For that to work, a single call to a
is_*_feature_detectedmacro must be able to accept multiple target features. I can see two ways to do that:is_x86_feature_detected("avx2", "bmi")is_x86_feature_detected("avx2,bmi")Option 2 has precedent in e.g.
#[target_feature(enable = "avx2,bmi2")], but option 1 can (I believe) be implemented withmacro_rules!and also works better with e.g.#[cfg(...)]. I personally prefer option 1.Alternatives
There is a workaround:
Links and related work
What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution: