[PATCH] D73667: Speed up compilation of ASTImporter
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaf3e88495627: Speed up compilation of ASTImporter (authored
by rnk).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
Files:
clang/lib/AST/ASTImporter.cpp
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Thanks for the patch! Looks good to me!
Comment at: clang/lib/AST/ASTImporter.cpp:192
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
Thanks for thinking about it. This way we can keep the original behavior i.e.
the import is stopped on the first error.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
merge_guards_bot added a comment.
{icon times-circle color=red} Unit tests: fail. 62308 tests passed, 1 failed
and 838 were skipped.
failed: libc++.std/thread/futures/futures_async/async.pass.cpp
{icon check-circle color=green} clang-tidy: pass.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
merge_guards_bot added a comment.
{icon check-circle color=green} Unit tests: pass. 62309 tests passed, 0 failed
and 838 were skipped.
{icon check-circle color=green} clang-tidy: pass.
{icon check-circle color=green} clang-format: pass.
Build artifacts
:
diff.json
,
clang-tidy.txt
,
clang-format.patch
,
CMakeCache.txt
,
console-log.txt
,
test-results.xml
//Pre-merge checks is in beta. Report issue
.
Please join beta .//
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
rnk added a comment.
The `Expected>` and `std::tie` instantiations are
still expensive, and they seem avoidable. We could use this code pattern
instead:
ExpectedType
ASTNodeImporter::VisitVariableArrayType(const VariableArrayType *T) {
QualType ToElementType = T->getElementType();
Expr *ToSizeExpr = T->getSizeExpr();
SourceRange ToBracketsRange = T->getBracketsRange();
if (Error E = importSeq(ToElementType, ToSizeExpr, ToBracketsRange))
return E;
return Importer.getToContext().getVariableArrayType(
ToElementType, ToSizeExpr, T->getSizeModifier(),
T->getIndexTypeCVRQualifiers(), ToBracketsRange);
}
Compare to the current pattern:
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTImporter.cpp#L1176
`importSeq` would take its parameters by reference. One by one, it would
replace them in place, or report an error.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
rnk updated this revision to Diff 241297.
rnk added a comment.
Herald added a subscriber: rnkovacs.
- remove unintended hack to test MSVC
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73667/new/
https://reviews.llvm.org/D73667
Files:
clang/lib/AST/ASTImporter.cpp
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D73667: Speed up compilation of ASTImporter
rnk created this revision.
rnk added reviewers: rsmith, aaron.ballman.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added subscribers: llvm-commits, teemperor.
Herald added projects: clang, LLVM.
rnk updated this revision to Diff 241297.
rnk added a comment.
Herald added a subscriber: rnkovacs.
- remove unintended hack to test MSVC
Avoid recursively instantiating importSeq. Use initializer list
expansion to stamp out a single instantiation of std::tuple of the
deduced sequence of types, and thread the error around that tuple type.
Avoids needlessly instantiating std::tuple N-1 times.
new time to compile: 0m25.985s
old time to compile: 0m35.563s
new obj size: 10,000kb
old obj size: 12,332kb
I found the slow TU by looking at ClangBuildAnalyzer results, and looked
at -ftime-trace for the file in chrome://tracing to find this.
Tested with: clang-cl, MSVC, and GCC.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D73667
Files:
clang/lib/AST/ASTImporter.cpp
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -186,32 +186,33 @@
return import(*From);
}
-template
-Expected>
-importSeq(const T ) {
- Expected ToOrErr = import(From);
- if (!ToOrErr)
-return ToOrErr.takeError();
- return std::make_tuple(std::move(*ToOrErr));
+// Helper for error management in importSeq.
+template T checkImport(Error , const T ) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
+return T{};
+ Expected MaybeVal = import(From);
+ if (!MaybeVal) {
+Err = MaybeVal.takeError();
+return T{};
+ }
+ return *MaybeVal;
}
// Import multiple objects with a single function call.
// This should work for every type for which a variant of `import` exists.
// The arguments are processed from left to right and import is stopped on
// first error.
-template
-Expected>
-importSeq(const THead , const TTail &...FromTail) {
- Expected> ToHeadOrErr = importSeq(FromHead);
- if (!ToHeadOrErr)
-return ToHeadOrErr.takeError();
- Expected> ToTailOrErr = importSeq(FromTail...);
- if (!ToTailOrErr)
-return ToTailOrErr.takeError();
- return std::tuple_cat(*ToHeadOrErr, *ToTailOrErr);
+template
+Expected> importSeq(const Args &... args) {
+ Error E = Error::success();
+ std::tuple Res{checkImport(E, args)...};
+ if (E)
+return std::move(E);
+ return std::move(Res);
}
-// Wrapper for an overload set.
+// Wrapper for an overload set.
template struct CallOverloadedCreateFun {
template
auto operator()(Args &&... args)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
