[clang][AST] Add 'IgnoreTemplateParmDepth' to structural equivalence cache #115518
Conversation
…cache Structural equivalence check uses a cache to store already found non-equivalent values. This cache can be reused for calls (ASTImporter does this). Value of "IgnoreTemplateParmDepth" can have an effect on the structural equivalence therefore it is wrong to reuse the same cache for checks with different values of 'IgnoreTemplateParmDepth'. The current change adds the 'IgnoreTemplateParmDepth' to the cache key to fix the problem.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Balázs Kéri (balazske) ChangesStructural equivalence check uses a cache to store already found non-equivalent values. This cache can be reused for calls (ASTImporter does this). Value of "IgnoreTemplateParmDepth" can have an effect on the structural equivalence therefore it is wrong to reuse the same cache for checks with different values of 'IgnoreTemplateParmDepth'. The current change adds the 'IgnoreTemplateParmDepth' to the cache key to fix the problem. Full diff: https://github.com/llvm/llvm-project/pull/115518.diff 7 Files Affected:
diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h
index 088a2bd0fdd407..8c3fa842ab8b9d 100644
--- a/clang/include/clang/AST/ASTImporter.h
+++ b/clang/include/clang/AST/ASTImporter.h
@@ -62,7 +62,8 @@ class TypeSourceInfo;
class ASTImporter {
friend class ASTNodeImporter;
public:
- using NonEquivalentDeclSet = llvm::DenseSet<std::pair<Decl *, Decl *>>;
+ using NonEquivalentDeclSet =
+ llvm::DenseSet<std::tuple<Decl *, Decl *, int>>;
using ImportedCXXBaseSpecifierMap =
llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
diff --git a/clang/include/clang/AST/ASTStructuralEquivalence.h b/clang/include/clang/AST/ASTStructuralEquivalence.h
index 029439c8e9a3ac..67aa0023c25d08 100644
--- a/clang/include/clang/AST/ASTStructuralEquivalence.h
+++ b/clang/include/clang/AST/ASTStructuralEquivalence.h
@@ -39,6 +39,10 @@ enum class StructuralEquivalenceKind {
};
struct StructuralEquivalenceContext {
+ /// Store declaration pairs already found to be non-equivalent.
+ /// key: (from, to, IgnoreTemplateParmDepth)
+ using NonEquivalentDeclSet = llvm::DenseSet<std::tuple<Decl *, Decl *, int>>;
+
/// AST contexts for which we are checking structural equivalence.
ASTContext &FromCtx, &ToCtx;
@@ -52,7 +56,7 @@ struct StructuralEquivalenceContext {
/// Declaration (from, to) pairs that are known not to be equivalent
/// (which we have already complained about).
- llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls;
+ NonEquivalentDeclSet &NonEquivalentDecls;
StructuralEquivalenceKind EqKind;
@@ -72,12 +76,13 @@ struct StructuralEquivalenceContext {
/// Whether to ignore comparing the depth of template param(TemplateTypeParm)
bool IgnoreTemplateParmDepth;
- StructuralEquivalenceContext(
- ASTContext &FromCtx, ASTContext &ToCtx,
- llvm::DenseSet<std::pair<Decl *, Decl *>> &NonEquivalentDecls,
- StructuralEquivalenceKind EqKind, bool StrictTypeSpelling = false,
- bool Complain = true, bool ErrorOnTagTypeMismatch = false,
- bool IgnoreTemplateParmDepth = false)
+ StructuralEquivalenceContext(ASTContext &FromCtx, ASTContext &ToCtx,
+ NonEquivalentDeclSet &NonEquivalentDecls,
+ StructuralEquivalenceKind EqKind,
+ bool StrictTypeSpelling = false,
+ bool Complain = true,
+ bool ErrorOnTagTypeMismatch = false,
+ bool IgnoreTemplateParmDepth = false)
: FromCtx(FromCtx), ToCtx(ToCtx), NonEquivalentDecls(NonEquivalentDecls),
EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling),
ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain),
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index 120ddc0f26c0d7..bf2f42932f25d4 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -2303,7 +2303,8 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
// Check whether we already know that these two declarations are not
// structurally equivalent.
- if (Context.NonEquivalentDecls.count(P))
+ if (Context.NonEquivalentDecls.count(
+ std::make_tuple(D1, D2, Context.IgnoreTemplateParmDepth)))
return false;
// Check if a check for these declarations is already pending.
@@ -2511,7 +2512,8 @@ bool StructuralEquivalenceContext::Finish() {
if (!Equivalent) {
// Note that these two declarations are not equivalent (and we already
// know about it).
- NonEquivalentDecls.insert(P);
+ NonEquivalentDecls.insert(
+ std::make_tuple(D1, D2, IgnoreTemplateParmDepth));
return true;
}
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index e526a11973975d..e26d422a31b49e 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9043,7 +9043,7 @@ bool Sema::RequireCompleteType(SourceLocation Loc, QualType T,
}
bool Sema::hasStructuralCompatLayout(Decl *D, Decl *Suggested) {
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls;
if (!Suggested)
return false;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 79615dc3c018ea..649bc318fa14c4 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9905,7 +9905,7 @@ void ASTReader::finishPendingActions() {
auto ExtensionsPair = PendingObjCExtensionIvarRedeclarations.back().first;
auto DuplicateIvars =
PendingObjCExtensionIvarRedeclarations.back().second;
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls;
StructuralEquivalenceContext Ctx(
ExtensionsPair.first->getASTContext(),
ExtensionsPair.second->getASTContext(), NonEquivalentDecls,
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 33fcddbbdb2f15..6ece3ba7af9f4b 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4447,7 +4447,7 @@ namespace {
ObjCCategoryDecl *&Existing = NameCategoryMap[Cat->getDeclName()];
if (Existing && Reader.getOwningModuleFile(Existing) !=
Reader.getOwningModuleFile(Cat)) {
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls;
StructuralEquivalenceContext Ctx(
Cat->getASTContext(), Existing->getASTContext(),
NonEquivalentDecls, StructuralEquivalenceKind::Default,
diff --git a/clang/unittests/AST/StructuralEquivalenceTest.cpp b/clang/unittests/AST/StructuralEquivalenceTest.cpp
index e994086c99d041..7cf52df9b14d05 100644
--- a/clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ b/clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -134,8 +134,8 @@ struct StructuralEquivalenceTest : ::testing::Test {
bool testStructuralMatch(Decl *D0, Decl *D1,
bool IgnoreTemplateParmDepth = false) {
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls01;
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls10;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls01;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls10;
StructuralEquivalenceContext Ctx01(
D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls01,
StructuralEquivalenceKind::Default, /*StrictTypeSpelling=*/false,
@@ -153,8 +153,8 @@ struct StructuralEquivalenceTest : ::testing::Test {
}
bool testStructuralMatch(StmtWithASTContext S0, StmtWithASTContext S1) {
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls01;
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls10;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls01;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls10;
StructuralEquivalenceContext Ctx01(
*S0.Context, *S1.Context, NonEquivalentDecls01,
StructuralEquivalenceKind::Default, false, false);
@@ -1792,7 +1792,7 @@ TEST_F(
EXPECT_FALSE(testStructuralMatch(t));
}
struct StructuralEquivalenceCacheTest : public StructuralEquivalenceTest {
- llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls;
template <typename NodeType, typename MatcherType>
std::pair<NodeType *, NodeType *>
@@ -1804,8 +1804,10 @@ struct StructuralEquivalenceCacheTest : public StructuralEquivalenceTest {
}
template <typename NodeType>
- bool isInNonEqCache(std::pair<NodeType *, NodeType *> D) {
- return NonEquivalentDecls.count(D) > 0;
+ bool isInNonEqCache(std::pair<NodeType *, NodeType *> D,
+ bool IgnoreTemplateParmDepth = false) {
+ return NonEquivalentDecls.count(
+ std::make_tuple(D.first, D.second, IgnoreTemplateParmDepth)) > 0;
}
};
@@ -2015,6 +2017,78 @@ TEST_F(StructuralEquivalenceCacheTest, Cycle) {
findDeclPair<FunctionDecl>(TU, functionDecl(hasName("x")))));
}
+TEST_F(StructuralEquivalenceCacheTest, TemplateParmDepth) {
+ // In 'friend struct Y' ClassTemplateDecl has the TU as parent context.
+ // This declaration has template depth 1 (it is already inside a template).
+ // It has not a previous declaration and is an "undeclared" friend.
+ //
+ // Second TU has a specialization of 'struct X'.
+ // In this case 'friend struct Y' has the ClassTemplateSpecializationDecl as
+ // parent. It has template depth 0 (it is in the specialization). It has the
+ // first 'struct Y' declaration as previous declaration and canonical
+ // declaration.
+ //
+ // When these two 'friend struct Y' are compared, only the template depth is
+ // different.
+ // FIXME: Structural equivalence checks the depth only in types, not in
+ // TemplateParmDecl. For this reason the second 'A1' argument is needed (as a
+ // type) in the template to make the check fail.
+ auto TU = makeTuDecls(
+ R"(
+ template <class A1, A1>
+ struct Y;
+
+ template <class A>
+ struct X {
+ template <class A1, A1>
+ friend struct Y;
+ };
+ )",
+ R"(
+ template <class A1, A1>
+ struct Y;
+
+ template <class A>
+ struct X {
+ template <class A1, A1>
+ friend struct Y;
+ };
+
+ X<int> x;
+ )",
+ Lang_CXX03);
+
+ auto *D0 = LastDeclMatcher<ClassTemplateDecl>().match(
+ get<0>(TU), classTemplateDecl(hasName("Y"), unless(isImplicit())));
+ auto *D1 = LastDeclMatcher<ClassTemplateDecl>().match(
+ get<1>(TU), classTemplateDecl(hasName("Y"), unless(isImplicit())));
+ ASSERT_EQ(D0->getTemplateDepth(), 1u);
+ ASSERT_EQ(D1->getTemplateDepth(), 0u);
+
+ StructuralEquivalenceContext Ctx_NoIgnoreTemplateParmDepth(
+ get<0>(TU)->getASTContext(), get<1>(TU)->getASTContext(),
+ NonEquivalentDecls, StructuralEquivalenceKind::Default, false, false,
+ false, false);
+
+ EXPECT_FALSE(Ctx_NoIgnoreTemplateParmDepth.IsEquivalent(D0, D1));
+
+ Decl *NonEqDecl0 =
+ D0->getCanonicalDecl()->getTemplateParameters()->getParam(1);
+ Decl *NonEqDecl1 =
+ D1->getCanonicalDecl()->getTemplateParameters()->getParam(1);
+ EXPECT_TRUE(isInNonEqCache(std::make_pair(NonEqDecl0, NonEqDecl1), false));
+ EXPECT_FALSE(isInNonEqCache(std::make_pair(NonEqDecl0, NonEqDecl1), true));
+
+ StructuralEquivalenceContext Ctx_IgnoreTemplateParmDepth(
+ get<0>(TU)->getASTContext(), get<1>(TU)->getASTContext(),
+ NonEquivalentDecls, StructuralEquivalenceKind::Default, false, false,
+ false, true);
+
+ EXPECT_TRUE(Ctx_IgnoreTemplateParmDepth.IsEquivalent(D0, D1));
+
+ EXPECT_FALSE(isInNonEqCache(std::make_pair(NonEqDecl0, NonEqDecl1), true));
+}
+
struct StructuralEquivalenceStmtTest : StructuralEquivalenceTest {};
/// Fallback matcher to be used only when there is no specific matcher for a
|
|
Another possible solution: Use two |
I guess another alternative would be to not cache equivalence of friends (I think that's the only time we ignore the template depth?). But that might not be desirable either. Your current approach seems reasonable to me |
|
I had another simple fix when the cache is used only if |
There was a problem hiding this comment.
Another possible solution: Use two
NonEquivalentDeclssets, one forIgnoreTemplateParmDepth = trueand one forfalse. This may use less memory (no rarely used third value in the key) but requires more code changes.
I like this idea, and I think it can be implemented without significant code changes -- see my inline comments for details.
(Disclaimer: I didn't check my change so there might be other locations where you didn't need to change between the old "set of pairs" and your "set of 3-tuples", but the suggested "two sets of pairs in an array" would behave differently -- but it would be a simplification even if you needed one or two lines of additional logic.)
However the current "set of 3-tuples" approach is also OK if you prefer it.
| using NonEquivalentDeclSet = | ||
| llvm::DenseSet<std::tuple<Decl *, Decl *, int>>; |
There was a problem hiding this comment.
| using NonEquivalentDeclSet = | |
| llvm::DenseSet<std::tuple<Decl *, Decl *, int>>; | |
| using NonEquivalentDeclSet = | |
| std::array<llvm::DenseSet<std::pair<Decl *, Decl *>>, 2>; |
If you use two DenseSets and put them into an array, you can use them without significant code changes.
| @@ -39,6 +39,10 @@ enum class StructuralEquivalenceKind { | |||
| }; | |||
|
|
|||
| struct StructuralEquivalenceContext { | |||
| /// Store declaration pairs already found to be non-equivalent. | |||
| /// key: (from, to, IgnoreTemplateParmDepth) | |||
| using NonEquivalentDeclSet = llvm::DenseSet<std::tuple<Decl *, Decl *, int>>; | |||
There was a problem hiding this comment.
| using NonEquivalentDeclSet = llvm::DenseSet<std::tuple<Decl *, Decl *, int>>; | |
| using NonEquivalentDeclSet = | |
| std::array<llvm::DenseSet<std::pair<Decl *, Decl *>>, 2>; |
As above.
| if (Context.NonEquivalentDecls.count( | ||
| std::make_tuple(D1, D2, Context.IgnoreTemplateParmDepth))) |
There was a problem hiding this comment.
| if (Context.NonEquivalentDecls.count( | |
| std::make_tuple(D1, D2, Context.IgnoreTemplateParmDepth))) | |
| if (Context.NonEquivalentDecls[Context.IgnoreTemplateParmDepth].count(P)) |
You can use a boolean value for indexing to select the right element in the array.
| NonEquivalentDecls.insert( | ||
| std::make_tuple(D1, D2, IgnoreTemplateParmDepth)); |
There was a problem hiding this comment.
| NonEquivalentDecls.insert( | |
| std::make_tuple(D1, D2, IgnoreTemplateParmDepth)); | |
| NonEquivalentDecls[IgnoreTemplateParmDepth].insert(P); |
| return NonEquivalentDecls.count( | ||
| std::make_tuple(D.first, D.second, IgnoreTemplateParmDepth)) > 0; |
There was a problem hiding this comment.
| return NonEquivalentDecls.count( | |
| std::make_tuple(D.first, D.second, IgnoreTemplateParmDepth)) > 0; | |
| return NonEquivalentDecls[IgnoreTemplateParmDepth].count(D) > 0; |
One thing I don't like about having two separate caches is the prospect of what this looks like if we add another parameter affecting the equivalence. For example, we already have another parameter |
|
It is a realistic requirement that new similar parameters are added, I had already an (experimental) fix where this is needed. Still I like better the solution with separate caches because efficiency reasons ( |
We wouldn't just have to add a single extra cache, but each parameter would require us to add caches for all permutations of the parameters. Also, I'd be surprised if there's a noticeable difference in performance between the two to be honest. So I still prefer your first solution, but I don't want to block this if you prefer the other approach |
|
The array-based solution can be extended to cover more than two separate cache sets by replacing the EDIT:
I agree that performance is not too important here; I'm favoring the array-based solution, because IMO it's more elegant that we don't need to re-pack the elements of the pair into a 3-tuple. Also note that, as the set of my inline comments shows, it's really easy to convert between the two approaches if the evolution of the codebase makes that necessary -- there is no need to aggressively future-proof this decision. Finally, this is all just bikeshedding and both potential solutions are better than spending more review rounds on this discussion. I'm fine with either implementation, let's merge one of them! |
|
OK, I will merge the current version. Even then it is possible to use the third tuple member with bitwise-OR combined additional flags. |
Structural equivalence check uses a cache to store already found non-equivalent values. This cache can be reused for calls (ASTImporter does this). Value of "IgnoreTemplateParmDepth" can have an effect on the structural equivalence therefore it is wrong to reuse the same cache for checks with different values of 'IgnoreTemplateParmDepth'. The current change adds the 'IgnoreTemplateParmDepth' to the cache key to fix the problem.