Load lazily the template specialization in multi-module setups.

[v.g.vassilev]

Summary

Currently, we load all template specialization if we have more than one module attached and we touch anything around the template definition.

This patch registers the template specializations as a lazily-loadable entities. This reduces the amount of deserializations by 1%.

[aprantl]

lib/Serialization/ASTReaderDecl.cpp

3814

llvm_unreachable()

[v.g.vassilev]

Reduce code duplication. Use llvm_unreachable.

[rsmith]

lib/Serialization/ASTReaderDecl.cpp

212–214

Can this ever fail at runtime? I'd expect the below code to not compile if D isn't one of these types.

3791–3816

This will end up being quadratic time and using quadratic storage if a module adds N specializations to a template; instead, please move the SmallVector out of the loop and call AddLazySpecializations once after reading all the update records for the declaration. (It's probably best to move it all the way out to loadDeclUpdateRecords, in case we have a large number of modules each adding some specializations -- eg, if module A imports std::vector and declares A, module B declares B and uses vector<A>, module C declares C and uses vector<B>, ..., we again should avoid quadratic behavior.)

[v.g.vassilev]

Reduce the amount of calls to AddLazySpecializations. Remove assert.

In order to create a reasonable test I need to use -error-on-deserialized-decl and I hit a bug: https://bugs.llvm.org/show_bug.cgi?id=32988

Richard, could you help me out with the test? Maybe we could trigger this avoiding the abovementioned bug...

[rsmith]

include/clang/Serialization/ASTReader.h

551 ↗

(On Diff #98445)

I'm not sure we can safely use global state for this: loading update records can trigger the import of a declaration and *its* update records, which would mean we'd attach the pending lazy specializations to the wrong declaration. Can you keep this list locally in ASTReader::loadDeclUpdateRecords instead, and pass it into UpdateDecl?

1293–1295 ↗

(On Diff #98445)

Make this a static member of ASTDeclReader to avoid this hack.

[rsmith]

In D29951#750885, @v.g.vassilev wrote:

In order to create a reasonable test I need to use -error-on-deserialized-decl and I hit a bug: https://bugs.llvm.org/show_bug.cgi?id=32988

Richard, could you help me out with the test? Maybe we could trigger this avoiding the abovementioned bug...

You could try to test this using -dump-deserialized-decls instead, but it'd be good to figure out what's going wrong in the above bug :) Nothing in the backtrace looks at all related to -dump-deserialized-decls, but maybe we're messing up our internal state by calling into the DeserializationListeners at an unsafe point during deserialization?

[v.g.vassilev]

Address comments.

[rsmith]

Looks great, thanks!

[v.g.vassilev]

r305120.

[v.g.vassilev]

Reverted in r305460 because broke libcxx modules builds.

[v.g.vassilev]

For the record: relanded in r306903.

lib/Serialization/ASTReaderDecl.cpp

class ASTDeclReader : public DeclVisitor<ASTDeclReader, void> {
     static DeclContext *getPrimaryContextForMerging(ASTReader &Reader,
                                                     DeclContext *DC);
     FindExistingResult findExisting(NamedDecl *D);
 
   public:
     ASTDeclReader(ASTReader &Reader, ASTRecordReader &Record,
                   ASTReader::RecordLocation Loc,
                   DeclID thisDeclID, SourceLocation ThisDeclLoc)
         : Reader(Reader), Record(Record), Loc(Loc),
           ThisDeclID(thisDeclID), ThisDeclLoc(ThisDeclLoc),
           TypeIDForTypeDecl(0), NamedDeclForTagDecl(0),
           TypedefNameForLinkage(nullptr), HasPendingBody(false),
           IsDeclMarkedUsed(false) {}
 
     template <typename DeclT>
     static Decl *getMostRecentDeclImpl(Redeclarable<DeclT> *D);
     static Decl *getMostRecentDeclImpl(...);
     static Decl *getMostRecentDecl(Decl *D);
 
case UPD_CXX_ADDED_IMPLICIT_MEMBER: {
       Decl *MD = Record.readDecl();
       assert(MD && "couldn't read decl from update record");
       // FIXME: We should call addHiddenDecl instead, to add the member
       // to its DeclContext.
       RD->addedMember(MD);
       break;
     }
 
-    case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION:
-      // It will be added to the template's specializations set when loaded.
-      (void)Record.readDecl();
+    case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION: {
+      // It will be added to the template's lazy specialization set when loaded.
+      // FIXME: reduce the copy paste.
+      SmallVector<serialization::DeclID, 1> SpecID;
+      if (auto *CTD = dyn_cast<ClassTemplateDecl>(D)) {
+        SpecID.push_back(ReadDeclID());
+        auto *CommonPtr = CTD->getCommonPtr();
+        CommonPtr->LazySpecializations = newDeclIDList(
+            Reader.getContext(), CommonPtr->LazySpecializations, SpecID);
+
+      } else if (auto *FTD = dyn_cast<FunctionTemplateDecl>(D)) {
+        SpecID.push_back(ReadDeclID());
+        auto *CommonPtr = FTD->getCommonPtr();
+        CommonPtr->LazySpecializations = newDeclIDList(
+            Reader.getContext(), CommonPtr->LazySpecializations, SpecID);
+
+      } else if (auto *VTD = dyn_cast<VarTemplateDecl>(D)) {
+        SpecID.push_back(ReadDeclID());
+        auto *CommonPtr = VTD->getCommonPtr();
+        CommonPtr->LazySpecializations = newDeclIDList(
+            Reader.getContext(), CommonPtr->LazySpecializations, SpecID);
+
+      } else // TypeAliasTemplateDecl
+        assert(0 && "TypeAliasTemplateDecl doesn't have specs!");
+    }
       break;
 
     case UPD_CXX_ADDED_ANONYMOUS_NAMESPACE: {
       NamespaceDecl *Anon = ReadDeclAs<NamespaceDecl>();
 
       // Each module has its own anonymous namespace, which is disjoint from
       // any other module's anonymous namespaces, so don't attach the anonymous
       // namespace at all.
       if (!Record.isModule()) {