[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0768b0576a93: Avoid redundant work when computing vtable
vcall visibility (authored by tejohnson).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91676/new/
https://reviews.llvm.org/D91676
Files:
clang/lib/CodeGen/CGVTables.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/CodeGen/MicrosoftCXXABI.cpp
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1333,8 +1333,11 @@
/// a virtual function call could be made which ends up being dispatched to a
/// member function of this class. This scope can be wider than the
visibility
/// of the class itself when the class has a more-visible dynamic base class.
+ /// The client should pass in an empty Visited set, which is used to prevent
+ /// redundant recursive processing.
llvm::GlobalObject::VCallVisibility
- GetVCallVisibilityLevel(const CXXRecordDecl *RD);
+ GetVCallVisibilityLevel(const CXXRecordDecl *RD,
+ llvm::DenseSet &Visited);
/// Emit type metadata for the given vtable using the given layout.
void EmitVTableTypeMetadata(const CXXRecordDecl *RD,
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1294,8 +1294,16 @@
return !HasLTOVisibilityPublicStd(RD);
}
-llvm::GlobalObject::VCallVisibility
-CodeGenModule::GetVCallVisibilityLevel(const CXXRecordDecl *RD) {
+llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
+const CXXRecordDecl *RD, llvm::DenseSet &Visited) {
+ // If we have already visited this RD (which means this is a recursive call
+ // since the initial call should have an empty Visited set), return the max
+ // visibility. The recursive calls below compute the min between the result
+ // of the recursive call and the current TypeVis, so returning the max here
+ // ensures that it will have no effect on the current TypeVis.
+ if (!Visited.insert(RD).second)
+return llvm::GlobalObject::VCallVisibilityTranslationUnit;
+
LinkageInfo LV = RD->getLinkageAndVisibility();
llvm::GlobalObject::VCallVisibility TypeVis;
if (!isExternallyVisible(LV.getLinkage()))
@@ -1307,13 +1315,15 @@
for (auto B : RD->bases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
for (auto B : RD->vbases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
return TypeVis;
}
@@ -1382,7 +1392,9 @@
if (getCodeGenOpts().VirtualFunctionElimination ||
getCodeGenOpts().WholeProgramVTables) {
-llvm::GlobalObject::VCallVisibility TypeVis = GetVCallVisibilityLevel(RD);
+llvm::DenseSet Visited;
+llvm::GlobalObject::VCallVisibility TypeVis =
+GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
wmi accepted this revision. wmi added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; + tejohnson wrote: > wmi wrote: > > tejohnson wrote: > > > wmi wrote: > > > > If a CXXRecordDecl is visited twice, the visibility returned in the > > > > second visit could be larger than necessary. Will it change the final > > > > result? If it will, can we cache the visibility result got in the first > > > > visit instead of returning the max value? > > > The recursive callsites compute the std::min of the current TypeVis and > > > the one returned by the recursive call. So returning the max guarantees > > > that there is no effect on the current TypeVis. Let me know if the > > > comment can be clarified (that's what I meant by "so that it has no > > > effect on the min visibility computed below ...". Note that the initial > > > non-recursive invocation always has an empty Visited set. > > I see. That makes sense! I didn't understand the location meant by > > "computed below by the recursive caller." Your explanation "initial > > non-recursive invocation always has an empty Visited set" helps a lot. It > > means the immediate result of GetVCallVisibilityLevel may change, but the > > result for the initial invocation of the recursive call won't be changed. > I've tried to clarify the comment accordingly Thanks for making the comment more clear. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; + wmi wrote: > tejohnson wrote: > > wmi wrote: > > > If a CXXRecordDecl is visited twice, the visibility returned in the > > > second visit could be larger than necessary. Will it change the final > > > result? If it will, can we cache the visibility result got in the first > > > visit instead of returning the max value? > > The recursive callsites compute the std::min of the current TypeVis and the > > one returned by the recursive call. So returning the max guarantees that > > there is no effect on the current TypeVis. Let me know if the comment can > > be clarified (that's what I meant by "so that it has no effect on the min > > visibility computed below ...". Note that the initial non-recursive > > invocation always has an empty Visited set. > I see. That makes sense! I didn't understand the location meant by "computed > below by the recursive caller." Your explanation "initial non-recursive > invocation always has an empty Visited set" helps a lot. It means the > immediate result of GetVCallVisibilityLevel may change, but the result for > the initial invocation of the recursive call won't be changed. I've tried to clarify the comment accordingly Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson updated this revision to Diff 307404.
tejohnson added a comment.
Improve comment
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91676/new/
https://reviews.llvm.org/D91676
Files:
clang/lib/CodeGen/CGVTables.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/CodeGen/MicrosoftCXXABI.cpp
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1333,8 +1333,11 @@
/// a virtual function call could be made which ends up being dispatched to a
/// member function of this class. This scope can be wider than the
visibility
/// of the class itself when the class has a more-visible dynamic base class.
+ /// The client should pass in an empty Visited set, which is used to prevent
+ /// redundant recursive processing.
llvm::GlobalObject::VCallVisibility
- GetVCallVisibilityLevel(const CXXRecordDecl *RD);
+ GetVCallVisibilityLevel(const CXXRecordDecl *RD,
+ llvm::DenseSet &Visited);
/// Emit type metadata for the given vtable using the given layout.
void EmitVTableTypeMetadata(const CXXRecordDecl *RD,
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1294,8 +1294,16 @@
return !HasLTOVisibilityPublicStd(RD);
}
-llvm::GlobalObject::VCallVisibility
-CodeGenModule::GetVCallVisibilityLevel(const CXXRecordDecl *RD) {
+llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
+const CXXRecordDecl *RD, llvm::DenseSet &Visited) {
+ // If we have already visited this RD (which means this is a recursive call
+ // since the initial call should have an empty Visited set), return the max
+ // visibility. The recursive calls below compute the min between the result
+ // of the recursive call and the current TypeVis, so returning the max here
+ // ensures that it will have no effect on the current TypeVis.
+ if (!Visited.insert(RD).second)
+return llvm::GlobalObject::VCallVisibilityTranslationUnit;
+
LinkageInfo LV = RD->getLinkageAndVisibility();
llvm::GlobalObject::VCallVisibility TypeVis;
if (!isExternallyVisible(LV.getLinkage()))
@@ -1307,13 +1315,15 @@
for (auto B : RD->bases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
for (auto B : RD->vbases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
return TypeVis;
}
@@ -1382,7 +1392,9 @@
if (getCodeGenOpts().VirtualFunctionElimination ||
getCodeGenOpts().WholeProgramVTables) {
-llvm::GlobalObject::VCallVisibility TypeVis = GetVCallVisibilityLevel(RD);
+llvm::DenseSet Visited;
+llvm::GlobalObject::VCallVisibility TypeVis =
+GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/CodeGenModule.h
===
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; + tejohnson wrote: > wmi wrote: > > If a CXXRecordDecl is visited twice, the visibility returned in the second > > visit could be larger than necessary. Will it change the final result? If > > it will, can we cache the visibility result got in the first visit instead > > of returning the max value? > The recursive callsites compute the std::min of the current TypeVis and the > one returned by the recursive call. So returning the max guarantees that > there is no effect on the current TypeVis. Let me know if the comment can be > clarified (that's what I meant by "so that it has no effect on the min > visibility computed below ...". Note that the initial non-recursive > invocation always has an empty Visited set. I see. That makes sense! I didn't understand the location meant by "computed below by the recursive caller." Your explanation "initial non-recursive invocation always has an empty Visited set" helps a lot. It means the immediate result of GetVCallVisibilityLevel may change, but the result for the initial invocation of the recursive call won't be changed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; + wmi wrote: > If a CXXRecordDecl is visited twice, the visibility returned in the second > visit could be larger than necessary. Will it change the final result? If it > will, can we cache the visibility result got in the first visit instead of > returning the max value? The recursive callsites compute the std::min of the current TypeVis and the one returned by the recursive call. So returning the max guarantees that there is no effect on the current TypeVis. Let me know if the comment can be clarified (that's what I meant by "so that it has no effect on the min visibility computed below ...". Note that the initial non-recursive invocation always has an empty Visited set. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
wmi added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; + If a CXXRecordDecl is visited twice, the visibility returned in the second visit could be larger than necessary. Will it change the final result? If it will, can we cache the visibility result got in the first visit instead of returning the max value? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson added a comment. Adding another reviewer - @wmi can you take a look? This is a straightforward compile time fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility
tejohnson created this revision.
tejohnson added reviewers: pcc, ostannard.
Herald added a project: clang.
tejohnson requested review of this revision.
Add a Visited set to avoid repeatedly processing the same base classes
in complex class hierarchies. This cut down the compile time of one
source file from >12min to ~1min.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91676
Files:
clang/lib/CodeGen/CGVTables.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/CodeGen/MicrosoftCXXABI.cpp
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1333,8 +1333,11 @@
/// a virtual function call could be made which ends up being dispatched to a
/// member function of this class. This scope can be wider than the
visibility
/// of the class itself when the class has a more-visible dynamic base class.
+ /// The client should pass in an empty Visited set, which is used to prevent
+ /// redundant recursive processing.
llvm::GlobalObject::VCallVisibility
- GetVCallVisibilityLevel(const CXXRecordDecl *RD);
+ GetVCallVisibilityLevel(const CXXRecordDecl *RD,
+ llvm::DenseSet &Visited);
/// Emit type metadata for the given vtable using the given layout.
void EmitVTableTypeMetadata(const CXXRecordDecl *RD,
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1294,8 +1294,13 @@
return !HasLTOVisibilityPublicStd(RD);
}
-llvm::GlobalObject::VCallVisibility
-CodeGenModule::GetVCallVisibilityLevel(const CXXRecordDecl *RD) {
+llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
+const CXXRecordDecl *RD, llvm::DenseSet &Visited) {
+ // If we have already visited this RD, return the max visibility, so that it
+ // has no effect on the min visibility computed below by the recursive
caller.
+ if (!Visited.insert(RD).second)
+return llvm::GlobalObject::VCallVisibilityTranslationUnit;
+
LinkageInfo LV = RD->getLinkageAndVisibility();
llvm::GlobalObject::VCallVisibility TypeVis;
if (!isExternallyVisible(LV.getLinkage()))
@@ -1307,13 +1312,15 @@
for (auto B : RD->bases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
for (auto B : RD->vbases())
if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
- TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+ TypeVis = std::min(
+ TypeVis,
+ GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
return TypeVis;
}
@@ -1382,7 +1389,9 @@
if (getCodeGenOpts().VirtualFunctionElimination ||
getCodeGenOpts().WholeProgramVTables) {
-llvm::GlobalObject::VCallVisibility TypeVis = GetVCallVisibilityLevel(RD);
+llvm::DenseSet Visited;
+llvm::GlobalObject::VCallVisibility TypeVis =
+GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
// TODO: Should VirtualFunctionElimination also be supported here?
// See similar handling in CodeGenModule::EmitVTableTypeMetadata.
if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
VTable->setVCallVisibilityMetadata(TypeVis);
}
Index: clang/lib/CodeGen/CodeGenModule.h
==
