[MC] Speed up checkFeatures() (NFCI) #130936
Conversation
checkFeatures() currently goes through ApplyFeatureFlag(), which will also handle implied features. This is very slow -- just querying every feature once takes up 10% of a Rust hello world compile. However, if we only want to query whether certain features are set/unset, we can do so directly -- implied features have already been handled when the FeatureBitset was constructed. I've retained the existing handling for unrecognized features (print an error and ignore them).
|
@llvm/pr-subscribers-mc Author: Nikita Popov (nikic) ChangescheckFeatures() currently goes through ApplyFeatureFlag(), which will also handle implied features. This is very slow -- just querying every feature once takes up 10% of a Rust hello world compile. However, if we only want to query whether certain features are set/unset, we can do so directly -- implied features have already been handled when the FeatureBitset was constructed. I've retained the existing handling for unrecognized features (print an error and ignore them), though I find it somewhat dubious... Full diff: https://github.com/llvm/llvm-project/pull/130936.diff 1 Files Affected:
diff --git a/llvm/lib/MC/MCSubtargetInfo.cpp b/llvm/lib/MC/MCSubtargetInfo.cpp
index f59ec2f7c2602..815e092deef22 100644
--- a/llvm/lib/MC/MCSubtargetInfo.cpp
+++ b/llvm/lib/MC/MCSubtargetInfo.cpp
@@ -317,14 +317,20 @@ FeatureBitset MCSubtargetInfo::ApplyFeatureFlag(StringRef FS) {
bool MCSubtargetInfo::checkFeatures(StringRef FS) const {
SubtargetFeatures T(FS);
- FeatureBitset Set, All;
- for (std::string F : T.getFeatures()) {
- ::ApplyFeatureFlag(Set, F, ProcFeatures);
- if (F[0] == '-')
- F[0] = '+';
- ::ApplyFeatureFlag(All, F, ProcFeatures);
- }
- return (FeatureBits & All) == Set;
+ return all_of(T.getFeatures(), [this](const std::string &F) {
+ assert(SubtargetFeatures::hasFlag(F) &&
+ "Feature flags should start with '+' or '-'");
+ const SubtargetFeatureKV *FeatureEntry =
+ Find(SubtargetFeatures::StripFlag(F), ProcFeatures);
+ if (!FeatureEntry) {
+ errs() << "'" << F << "' is not a recognized feature for this target"
+ << " (ignoring feature)\n";
+ return true;
+ }
+
+ return FeatureBits.test(FeatureEntry->Value) ==
+ SubtargetFeatures::isEnabled(F);
+ });
}
const MCSchedModel &MCSubtargetInfo::getSchedModelForCPU(StringRef CPU) const {
|
llvm/lib/MC/MCSubtargetInfo.cpp
Outdated
| errs() << "'" << F << "' is not a recognized feature for this target" | ||
| << " (ignoring feature)\n"; |
There was a problem hiding this comment.
Is ApplyFeatureFlag dead now? Or is it used during the construction of the bitset? Can the warning be left for that? We should migrate this to using DiagnosticInfo at least
There was a problem hiding this comment.
ApplyFeatureFlag is still used during construction.
I've changed this code to report a fatal error on invalid feature now. I don't think there's a good reason to make this a warning in checkFeatures().
Until recently checkFeature was quite slow. llvm#130936 I was curious where we use checkFeature and noticed these. I thought we could use hasFeature instead of going through strings.
checkFeatures() currently goes through ApplyFeatureFlag(), which will also handle implied features. This is very slow -- just querying every feature once takes up 10% of a Rust hello world compile. However, if we only want to query whether certain features are set/unset, we can do so directly -- implied features have already been handled when the FeatureBitset was constructed.
checkFeatures() currently goes through ApplyFeatureFlag(), which will also handle implied features. This is very slow -- just querying every feature once takes up 10% of a Rust hello world compile.
However, if we only want to query whether certain features are set/unset, we can do so directly -- implied features have already been handled when the FeatureBitset was constructed.