-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Speed up target feature computation #137586
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
Changes from 3 commits
1df93fd
2df8e65
936a823
35b7994
cee3114
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -306,18 +306,18 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option<LLVMFea | |
/// Must express features in the way Rust understands them. | ||
/// | ||
/// We do not have to worry about RUSTC_SPECIFIC_FEATURES here, those are handled outside codegen. | ||
pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<Symbol> { | ||
pub(crate) fn target_features_cfg(sess: &Session) -> (Vec<Symbol>, Vec<Symbol>) { | ||
let mut features: FxHashSet<Symbol> = Default::default(); | ||
|
||
// Add base features for the target. | ||
// We do *not* add the -Ctarget-features there, and instead duplicate the logic for that below. | ||
// The reason is that if LLVM considers a feature implied but we do not, we don't want that to | ||
// show up in `cfg`. That way, `cfg` is entirely under our control -- except for the handling of | ||
// the target CPU, that is still expanded to target features (with all their implied features) by | ||
// LLVM. | ||
// the target CPU, that is still expanded to target features (with all their implied features) | ||
// by LLVM. | ||
let target_machine = create_informational_target_machine(sess, true); | ||
// Compute which of the known target features are enabled in the 'base' target machine. | ||
// We only consider "supported" features; "forbidden" features are not reflected in `cfg` as of now. | ||
// Compute which of the known target features are enabled in the 'base' target machine. We only | ||
// consider "supported" features; "forbidden" features are not reflected in `cfg` as of now. | ||
features.extend( | ||
sess.target | ||
.rust_target_features() | ||
|
@@ -331,6 +331,9 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<S | |
if let Some(feat) = to_llvm_features(sess, feature) { | ||
for llvm_feature in feat { | ||
let cstr = SmallCStr::new(llvm_feature); | ||
// `LLVMRustHasFeature` is moderately expensive. On targets with many | ||
// features (e.g. x86) these calls take a non-trivial fraction of runtime | ||
// when compiling very small programs. | ||
if !unsafe { llvm::LLVMRustHasFeature(target_machine.raw(), cstr.as_ptr()) } | ||
{ | ||
return false; | ||
|
@@ -344,7 +347,7 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<S | |
.map(|(feature, _, _)| Symbol::intern(feature)), | ||
); | ||
|
||
// Add enabled features | ||
// Add enabled and remove disabled features. | ||
for (enabled, feature) in | ||
sess.opts.cg.target_feature.split(',').filter_map(|s| match s.chars().next() { | ||
Some('+') => Some((true, Symbol::intern(&s[1..]))), | ||
|
@@ -360,7 +363,7 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<S | |
#[allow(rustc::potential_query_instability)] | ||
features.extend( | ||
sess.target | ||
.implied_target_features(std::iter::once(feature.as_str())) | ||
.implied_target_features(feature.as_str()) | ||
.iter() | ||
.map(|s| Symbol::intern(s)), | ||
); | ||
|
@@ -371,11 +374,7 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<S | |
// `features.contains` below. | ||
#[allow(rustc::potential_query_instability)] | ||
features.retain(|f| { | ||
if sess | ||
.target | ||
.implied_target_features(std::iter::once(f.as_str())) | ||
.contains(&feature.as_str()) | ||
{ | ||
if sess.target.implied_target_features(f.as_str()).contains(&feature.as_str()) { | ||
// If `f` if implies `feature`, then `!feature` implies `!f`, so we have to | ||
// remove `f`. (This is the standard logical contraposition principle.) | ||
false | ||
|
@@ -387,25 +386,31 @@ pub(crate) fn target_features_cfg(sess: &Session, allow_unstable: bool) -> Vec<S | |
} | ||
} | ||
|
||
// Filter enabled features based on feature gates | ||
sess.target | ||
.rust_target_features() | ||
.iter() | ||
.filter_map(|(feature, gate, _)| { | ||
// The `allow_unstable` set is used by rustc internally to determined which target | ||
// features are truly available, so we want to return even perma-unstable "forbidden" | ||
// features. | ||
if allow_unstable | ||
|| (gate.in_cfg() && (sess.is_nightly_build() || gate.requires_nightly().is_none())) | ||
{ | ||
Some(*feature) | ||
} else { | ||
None | ||
} | ||
}) | ||
.filter(|feature| features.contains(&Symbol::intern(feature))) | ||
.map(|feature| Symbol::intern(feature)) | ||
.collect() | ||
// Filter enabled features based on feature gates. | ||
let f = |allow_unstable| { | ||
sess.target | ||
.rust_target_features() | ||
.iter() | ||
.filter_map(|(feature, gate, _)| { | ||
// The `allow_unstable` set is used by rustc internally to determined which target | ||
// features are truly available, so we want to return even perma-unstable | ||
// "forbidden" features. | ||
if allow_unstable | ||
|| (gate.in_cfg() | ||
&& (sess.is_nightly_build() || gate.requires_nightly().is_none())) | ||
{ | ||
Some(Symbol::intern(feature)) | ||
} else { | ||
None | ||
} | ||
}) | ||
.filter(|feature| features.contains(&feature)) | ||
.collect() | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code in this closure is duplicated between all codegen backends. Maybe move it to the caller of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see it in the cranelift backend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And if the response is "the cranelift backend should do that filtering", that would also be a good follow-up, because that's a pre-existing issue that is distinct from the one this PR is addressing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Cranelift backend currently doesn't consider any unstable feature to ever be enabled at all, so filtering has no effect. Once it does support unstable features, yes it should do filtering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, makes sense. |
||
|
||
let target_features = f(false); | ||
let unstable_target_features = f(true); | ||
(target_features, unstable_target_features) | ||
} | ||
|
||
pub(crate) fn print_version() { | ||
|
@@ -682,7 +687,7 @@ pub(crate) fn global_llvm_features( | |
for feature in sess.opts.cg.target_feature.split(',') { | ||
if let Some(feature) = feature.strip_prefix('+') { | ||
all_rust_features.extend( | ||
UnordSet::from(sess.target.implied_target_features(std::iter::once(feature))) | ||
UnordSet::from(sess.target.implied_target_features(feature)) | ||
.to_sorted_stable_ord() | ||
.iter() | ||
.map(|&&s| (true, s)), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not just unstable target features as the name of this variable implies, but all including unstable ones. Maybe rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all_target_features
andstable_target_features
would be clearer names, yes. ButSession
already has fields calledunstable_target_features
andtarget_features
, and this is the code that is used to set those fields, so I used those names for consistency. If you think it's important, maybe it should be a follow-up.