llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Jonas Hahnfeld (hahnjo) <details> <summary>Changes</summary> * Hash inner template arguments: The code is applied from `ODRHash::AddDecl` with the reasoning given in the comment, to reduce collisions. This was particularly visible with STL types templated on `std::pair` where its template arguments were not taken into account. * Complete only needed partial specializations: It is unclear (to me) why this needs to be done "for safety", but this change significantly improves the effectiveness of lazy loading. * Load only needed partial specializations: Similar as the last commit, it is unclear why we need to load all specializations, including non-partial ones, when we have a `TPL`. * Remove bail-out logic in `TemplateArgumentHasher`: While it is correct to assign a single fixed hash to all template arguments, it can reduce the effectiveness of lazy loading and is not actually needed: we are allowed to ignore parts that cannot be handled because they will be analogously ignored by all hashings. --- Full diff: https://github.com/llvm/llvm-project/pull/133057.diff 3 Files Affected: - (modified) clang/lib/AST/DeclTemplate.cpp (-6) - (modified) clang/lib/Serialization/ASTReader.cpp (+2-8) - (modified) clang/lib/Serialization/TemplateArgumentHasher.cpp (+18-33) ``````````diff diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp index c0f5be51db5f3..8560c3928aa84 100644 --- a/clang/lib/AST/DeclTemplate.cpp +++ b/clang/lib/AST/DeclTemplate.cpp @@ -367,12 +367,6 @@ bool RedeclarableTemplateDecl::loadLazySpecializationsImpl( if (!ExternalSource) return false; - // If TPL is not null, it implies that we're loading specializations for - // partial templates. We need to load all specializations in such cases. - if (TPL) - return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(), - /*OnlyPartial=*/false); - return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(), Args); } diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 0cd2cedb48dd9..eb0496c97eb3b 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -7891,14 +7891,8 @@ void ASTReader::CompleteRedeclChain(const Decl *D) { } } - if (Template) { - // For partitial specialization, load all the specializations for safety. - if (isa<ClassTemplatePartialSpecializationDecl, - VarTemplatePartialSpecializationDecl>(D)) - Template->loadLazySpecializationsImpl(); - else - Template->loadLazySpecializationsImpl(Args); - } + if (Template) + Template->loadLazySpecializationsImpl(Args); } CXXCtorInitializer ** diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp b/clang/lib/Serialization/TemplateArgumentHasher.cpp index 3c7177b83ba52..5fb363c4ab148 100644 --- a/clang/lib/Serialization/TemplateArgumentHasher.cpp +++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp @@ -21,17 +21,6 @@ using namespace clang; namespace { class TemplateArgumentHasher { - // If we bail out during the process of calculating hash values for - // template arguments for any reason. We're allowed to do it since - // TemplateArgumentHasher are only required to give the same hash value - // for the same template arguments, but not required to give different - // hash value for different template arguments. - // - // So in the worst case, it is still a valid implementation to give all - // inputs the same BailedOutValue as output. - bool BailedOut = false; - static constexpr unsigned BailedOutValue = 0x12345678; - llvm::FoldingSetNodeID ID; public: @@ -41,14 +30,7 @@ class TemplateArgumentHasher { void AddInteger(unsigned V) { ID.AddInteger(V); } - unsigned getValue() { - if (BailedOut) - return BailedOutValue; - - return ID.computeStableHash(); - } - - void setBailedOut() { BailedOut = true; } + unsigned getValue() { return ID.computeStableHash(); } void AddType(const Type *T); void AddQualType(QualType T); @@ -92,8 +74,7 @@ void TemplateArgumentHasher::AddTemplateArgument(TemplateArgument TA) { case TemplateArgument::Expression: // If we meet expression in template argument, it implies // that the template is still dependent. It is meaningless - // to get a stable hash for the template. Bail out simply. - BailedOut = true; + // to get a stable hash for the template. break; case TemplateArgument::Pack: AddInteger(TA.pack_size()); @@ -110,10 +91,9 @@ void TemplateArgumentHasher::AddStructuralValue(const APValue &Value) { // 'APValue::Profile' uses pointer values to make hash for LValue and // MemberPointer, but they differ from one compiler invocation to another. - // It may be difficult to handle such cases. Bail out simply. + // It may be difficult to handle such cases. if (Kind == APValue::LValue || Kind == APValue::MemberPointer) { - BailedOut = true; return; } @@ -135,14 +115,11 @@ void TemplateArgumentHasher::AddTemplateName(TemplateName Name) { case TemplateName::DependentTemplate: case TemplateName::SubstTemplateTemplateParm: case TemplateName::SubstTemplateTemplateParmPack: - BailedOut = true; break; case TemplateName::UsingTemplate: { UsingShadowDecl *USD = Name.getAsUsingShadowDecl(); if (USD) AddDecl(USD->getTargetDecl()); - else - BailedOut = true; break; } case TemplateName::DeducedTemplate: @@ -167,7 +144,6 @@ void TemplateArgumentHasher::AddDeclarationName(DeclarationName Name) { case DeclarationName::ObjCZeroArgSelector: case DeclarationName::ObjCOneArgSelector: case DeclarationName::ObjCMultiArgSelector: - BailedOut = true; break; case DeclarationName::CXXConstructorName: case DeclarationName::CXXDestructorName: @@ -194,16 +170,29 @@ void TemplateArgumentHasher::AddDeclarationName(DeclarationName Name) { void TemplateArgumentHasher::AddDecl(const Decl *D) { const NamedDecl *ND = dyn_cast<NamedDecl>(D); if (!ND) { - BailedOut = true; return; } AddDeclarationName(ND->getDeclName()); + + // If this was a specialization we should take into account its template + // arguments. This helps to reduce collisions coming when visiting template + // specialization types (eg. when processing type template arguments). + ArrayRef<TemplateArgument> Args; + if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D)) + Args = CTSD->getTemplateArgs().asArray(); + else if (auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D)) + Args = VTSD->getTemplateArgs().asArray(); + else if (auto *FD = dyn_cast<FunctionDecl>(D)) + if (FD->getTemplateSpecializationArgs()) + Args = FD->getTemplateSpecializationArgs()->asArray(); + + for (auto &TA : Args) + AddTemplateArgument(TA); } void TemplateArgumentHasher::AddQualType(QualType T) { if (T.isNull()) { - BailedOut = true; return; } SplitQualType split = T.split(); @@ -213,7 +202,6 @@ void TemplateArgumentHasher::AddQualType(QualType T) { // Process a Type pointer. Add* methods call back into TemplateArgumentHasher // while Visit* methods process the relevant parts of the Type. -// Any unhandled type will make the hash computation bail out. class TypeVisitorHelper : public TypeVisitor<TypeVisitorHelper> { typedef TypeVisitor<TypeVisitorHelper> Inherited; llvm::FoldingSetNodeID &ID; @@ -245,9 +233,6 @@ class TypeVisitorHelper : public TypeVisitor<TypeVisitorHelper> { void Visit(const Type *T) { Inherited::Visit(T); } - // Unhandled types. Bail out simply. - void VisitType(const Type *T) { Hash.setBailedOut(); } - void VisitAdjustedType(const AdjustedType *T) { AddQualType(T->getOriginalType()); } `````````` </details> https://github.com/llvm/llvm-project/pull/133057 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
