-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[flang][NFC] speedup BoxedProcedure for derived types with many components #86144
Conversation
…nents This patch speeds up the compilation time of the example in llvm#76478 (comment) from 2mins with my builds to about 2 seconds. MLIR timers showed more than 98% of the time was spend in BoxedProcedure trying to figure out if a type needs to be converted. This is because walking the fir.type members is very expansive for types containing many components and/or components with many sub-components. Increase the caching time of visited types from "the type being visited" to "the whole pass". Use DenseMap since it is not ok anymore to assume this container will only have a few elements.
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesThis patch speeds up the compilation time of the example in #76478 (comment) from 2 minutes with my builds to about 2 seconds. MLIR timers showed more than 98% of the time was spend in BoxedProcedure trying to figure out if a type needs to be converted. This is because walking the fir.type members is very expansive for types containing many components and/or components with many sub-components. Increase the caching time of visited types from "the type being visited" to "the whole pass". Use DenseMap since it is not ok anymore to assume this container will only have a few elements. Full diff: https://github.com/llvm/llvm-project/pull/86144.diff 1 Files Affected:
diff --git a/flang/lib/Optimizer/CodeGen/BoxedProcedure.cpp b/flang/lib/Optimizer/CodeGen/BoxedProcedure.cpp
index 746c275f37eaca..7a1cc3536b263b 100644
--- a/flang/lib/Optimizer/CodeGen/BoxedProcedure.cpp
+++ b/flang/lib/Optimizer/CodeGen/BoxedProcedure.cpp
@@ -69,17 +69,20 @@ class BoxprocTypeRewriter : public mlir::TypeConverter {
return false;
}
if (auto recTy = ty.dyn_cast<RecordType>()) {
- if (llvm::is_contained(visitedTypes, recTy))
- return false;
+ auto visited = visitedTypes.find(ty);
+ if (visited != visitedTypes.end())
+ return visited->second;
+ [[maybe_unused]] auto newIt = visitedTypes.try_emplace(ty, false);
+ assert(newIt.second && "expected ty to not be in the map");
bool result = false;
- visitedTypes.push_back(recTy);
for (auto t : recTy.getTypeList()) {
if (needsConversion(t.second)) {
result = true;
break;
}
}
- visitedTypes.pop_back();
+ // newIt may have been invalidated.
+ visitedTypes.find(ty)->second = result;
return result;
}
if (auto boxTy = ty.dyn_cast<BaseBoxType>())
@@ -140,9 +143,7 @@ class BoxprocTypeRewriter : public mlir::TypeConverter {
if (rec.isFinalized())
return rec;
auto it = convertedTypes.try_emplace(ty, rec);
- if (!it.second) {
- llvm::errs() << "failed\n" << ty << "\n";
- }
+ assert(it.second && "expected ty to not be in the map");
std::vector<RecordType::TypePair> ps = ty.getLenParamList();
std::vector<RecordType::TypePair> cs;
for (auto t : ty.getTypeList()) {
@@ -171,11 +172,11 @@ class BoxprocTypeRewriter : public mlir::TypeConverter {
void setLocation(mlir::Location location) { loc = location; }
private:
- llvm::SmallVector<mlir::Type> visitedTypes;
- // Map to deal with recursive derived types (avoid infinite loops).
+ // Maps to deal with recursive derived types (avoid infinite loops).
// Caching is also beneficial for apps with big types (dozens of
// components and or parent types), so the lifetime of the cache
// is the whole pass.
+ llvm::DenseMap<mlir::Type, bool> visitedTypes;
llvm::DenseMap<mlir::Type, mlir::Type> convertedTypes;
mlir::Location loc;
};
|
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.
LGTM. Nice improvement!
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.
LGTM. This solves the case for both reproducers in #76478
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.
LGTM. Thank you for the quick fix!
The previous update caused t002 type in the added test to not be rewritten because it was first analyzed when analyzing t001. Since the reason it needs to be rewritten is because it contains a t001, but t001 is not fully analyzed when first analyzing t002, the first analyzed yieled false. This "wrong" result is not an issue when analyzing t001 (since t002 does not determine t001 result). But caching this invalid result for later usage is invalid. Add a boolean to only keep analysis for top-level types where the result is known to be correct. This restriction has no measurable impact on the speed-up: I still measure 2 seconds.
This patch speeds up the compilation time of the example in #76478 (comment) from 2 minutes with my builds to about 2 seconds.
MLIR timers showed more than 98% of the time was spend in BoxedProcedure trying to figure out if a type needs to be converted.
This is because walking the fir.type members is very expansive for types containing many components and/or components with many sub-components.
Increase the caching time of visited types from "the type being visited" to "the whole pass". Use DenseMap since it is not ok anymore to assume this container will only have a few elements.