Skip to content

[Serialization] Fix lazy template loading #133057

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+20 −47
Open

[Serialization] Fix lazy template loading #133057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
670d16c
[Serialization] Hash inner template arguments
hahnjo Mar 25, 2025
658d55b
[Serialization] Complete only needed partial specializations
hahnjo Mar 25, 2025
48f50c7
[Serialization] Load only needed partial specializations
hahnjo Mar 25, 2025
c8b3418
[Serialization] Remove bail-out logic in TemplateArgumentHasher
hahnjo Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions clang/lib/AST/DeclTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can remove TPL arguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could remove it from loadLazySpecializationsImpl, but we need it in Profile since df061c3. This complicates the removal considerably, which IMHO makes the code more complicated: hahnjo@bb4788a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking into df061c3 this may be something which is not properly handled for lazy template loading. @emaxx-google can you check if the failures you are seeing involves constrained partial specializations or function overloads? If so, can you mayble locally revert 48f50c7 [Serialization] Load only needed partial specializations?

return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
/*OnlyPartial=*/false);

return ExternalSource->LoadExternalSpecializations(this->getCanonicalDecl(),
Args);
}
Expand Down
10 changes: 2 additions & 8 deletions clang/lib/Serialization/ASTReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 **
Expand Down
51 changes: 18 additions & 33 deletions clang/lib/Serialization/TemplateArgumentHasher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines -24 to -33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a concern for this. If we have two semantically same Decl/Type but not literally same, if we give them different hash value, we will pay for additional specialization cost.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the hasher must skip all inputs that could differ in their literal spelling, but I don't see why we have to hash them to a single global value. If two semantically same Decls / Types are not literally the same, we skip the input for both of them and the output hash will still be the same.

Do you have a concrete example in mind that you think should be tested?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet. We can split this in two commits, then if anything bad happens, we can revert precisely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I can also land the four commits individually and link back with a Reviewed as part of in the commit messages.


llvm::FoldingSetNodeID ID;

public:
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand All @@ -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;
}

Expand All @@ -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:
Expand All @@ -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:
Expand All @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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());
}
Expand Down