[Serialization] [ASTWriter] Try to not record namespace as much as possible (#179178)

See

https://clang.llvm.org/docs/StandardCPlusPlusModules.html#experimental-non-cascading-changes
for the full background.

In short, we want to cut off the dependencies to improve the
recompilation time.

And namespace is special, as the same namespace can occur in many TUs.

This patch tries to clean up unneeded reference to namespace from other
module file. The touched parts are:

- When write on disk lookup table, don't write the merged table. This
makes the ownership/dependency much cleaner. For performance, I think
the intention was to make the cost of mergin table amortized. But in our
internal workloads, I didn't observe any regression if I turn off
writing the merged table.
- When writing redeclarations, only write the ID of the first namespace
and avoid referenece to all other previous namespaces. (I don't want to
write the first namespace either... but it is more complex).
- When writing the name lookup table, try to write the local namespace.

@jtstogel @mpark I want to invite you to test this with your internal
workloads to figure out the correctness and the performance impact on
this.

I know I can make the change clean for you by inserting code guarded by
"if writing named module" but I think it will be much better if we can
make the underlying implementation more homogeneous if possible.

clang/include/clang/Serialization/ASTWriter.h

@@ -928,8 +928,6 @@ public:
 
   void handleVTable(CXXRecordDecl *RD);
 
-  void addTouchedModuleFile(serialization::ModuleFile *);
-
 private:
   // ASTDeserializationListener implementation
   void ReaderInitialized(ASTReader *Reader) override;

clang/lib/Serialization/ASTWriter.cpp

@@ -4085,10 +4085,6 @@ void ASTWriter::handleVTable(CXXRecordDecl *RD) {
   PendingEmittingVTables.push_back(RD);
 }
 
-void ASTWriter::addTouchedModuleFile(serialization::ModuleFile *MF) {
-  TouchedModuleFiles.insert(MF);
-}
-
 //===----------------------------------------------------------------------===//
 // DeclContext's Name Lookup Table Serialization
 //===----------------------------------------------------------------------===//
@@ -4133,7 +4129,6 @@ public:
            "have reference to loaded module file but no chain?");
 
     using namespace llvm::support;
-    Writer.addTouchedModuleFile(F);
     endian::write<uint32_t>(Out, Writer.getChain()->getModuleFileID(F),
                             llvm::endianness::little);
   }
@@ -4331,6 +4326,14 @@ static bool isTULocalInNamedModules(NamedDecl *D) {
   return D->getLinkageInternal() == Linkage::Internal;
 }
 
+static NamedDecl *getLocalRedecl(NamedDecl *D) {
+  for (auto *RD : D->redecls())
+    if (!RD->isFromASTFile())
+      return cast<NamedDecl>(RD);
+
+  return D;
+}
+
 class ASTDeclContextNameLookupTrait
     : public ASTDeclContextNameTrivialLookupTrait {
 public:
@@ -4404,6 +4407,13 @@ public:
       NamedDecl *DeclForLocalLookup =
           getDeclForLocalLookup(Writer.getLangOpts(), D);
 
+      // Namespace may have many redeclarations in many TU.
+      // Also, these namespaces are symmetric. So that we try to use
+      // the local redecl if any.
+      if (isa<NamespaceDecl>(DeclForLocalLookup) &&
+          DeclForLocalLookup->isFromASTFile())
+        DeclForLocalLookup = getLocalRedecl(DeclForLocalLookup);
+
       if (Writer.getDoneWritingDeclsAndTypes() &&
           !Writer.wasDeclEmitted(DeclForLocalLookup))
         return;
@@ -4525,7 +4535,6 @@ public:
            "have reference to loaded module file but no chain?");
 
     using namespace llvm::support;
-    Writer.addTouchedModuleFile(F);
     endian::write<uint32_t>(Out, Writer.getChain()->getModuleFileID(F),
                             llvm::endianness::little);
   }
@@ -4624,7 +4633,16 @@ void ASTWriter::GenerateSpecializationInfoLookupTable(
     Generator.insert(HashValue, Trait.getData(Specs, ExisitingSpecs), Trait);
   }
 
-  Generator.emit(LookupTable, Trait, Lookups ? &Lookups->Table : nullptr);
+  // Reduced BMI may not emit everything in the lookup table,
+  // If Reduced BMI **partially** emits some decls,
+  // then the generator may not emit the corresponding entry for the
+  // corresponding name is already there. See
+  // MultiOnDiskHashTableGenerator::insert and
+  // MultiOnDiskHashTableGenerator::emit for details.
+  // So we won't emit the lookup table if we're generating reduced BMI.
+  auto *ToEmitMaybeMergedLookupTable =
+      (!isGeneratingReducedBMI() && Lookups) ? &Lookups->Table : nullptr;
+  Generator.emit(LookupTable, Trait, ToEmitMaybeMergedLookupTable);
 }
 
 uint64_t ASTWriter::WriteSpecializationInfoLookupTable(
@@ -4821,7 +4839,16 @@ void ASTWriter::GenerateNameLookupTable(
   // Create the on-disk hash table. Also emit the existing imported and
   // merged table if there is one.
   auto *Lookups = Chain ? Chain->getLoadedLookupTables(DC) : nullptr;
-  Generator.emit(LookupTable, Trait, Lookups ? &Lookups->Table : nullptr);
+  // Reduced BMI may not emit everything in the lookup table,
+  // If Reduced BMI **partially** emits some decls,
+  // then the generator may not emit the corresponding entry for the
+  // corresponding name is already there. See
+  // MultiOnDiskHashTableGenerator::insert and
+  // MultiOnDiskHashTableGenerator::emit for details.
+  // So we won't emit the lookup table if we're generating reduced BMI.
+  auto *ToEmitMaybeMergedLookupTable =
+      (!isGeneratingReducedBMI() && Lookups) ? &Lookups->Table : nullptr;
+  Generator.emit(LookupTable, Trait, ToEmitMaybeMergedLookupTable);
 
   const auto &ModuleLocalDecls = Trait.getModuleLocalDecls();
   if (!ModuleLocalDecls.empty()) {
@@ -4837,11 +4864,15 @@ void ASTWriter::GenerateNameLookupTable(
                                         ModuleLocalTrait);
     }
 
+    // See the above comment. We won't emit the merged table if we're generating
+    // reduced BMI.
     auto *ModuleLocalLookups =
-        Chain ? Chain->getModuleLocalLookupTables(DC) : nullptr;
-    ModuleLocalLookupGenerator.emit(
-        ModuleLocalLookupTable, ModuleLocalTrait,
-        ModuleLocalLookups ? &ModuleLocalLookups->Table : nullptr);
+        (isGeneratingReducedBMI() && Chain &&
+         Chain->getModuleLocalLookupTables(DC))
+            ? &Chain->getModuleLocalLookupTables(DC)->Table
+            : nullptr;
+    ModuleLocalLookupGenerator.emit(ModuleLocalLookupTable, ModuleLocalTrait,
+                                    ModuleLocalLookups);
   }
 
   const auto &TULocalDecls = Trait.getTULocalDecls();
@@ -4857,9 +4888,13 @@ void ASTWriter::GenerateNameLookupTable(
       TULookupGenerator.insert(Key, TULocalTrait.getData(IDs), TULocalTrait);
     }
 
-    auto *TULocalLookups = Chain ? Chain->getTULocalLookupTables(DC) : nullptr;
-    TULookupGenerator.emit(TULookupTable, TULocalTrait,
-                           TULocalLookups ? &TULocalLookups->Table : nullptr);
+    // See the above comment. We won't emit the merged table if we're generating
+    // reduced BMI.
+    auto *TULocalLookups =
+        (isGeneratingReducedBMI() && Chain && Chain->getTULocalLookupTables(DC))
+            ? &Chain->getTULocalLookupTables(DC)->Table
+            : nullptr;
+    TULookupGenerator.emit(TULookupTable, TULocalTrait, TULocalLookups);
   }
 }
 

clang/lib/Serialization/ASTWriterDecl.cpp

@@ -2234,8 +2234,15 @@ void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) {
       // redecl chain.
       unsigned I = Record.size();
       Record.push_back(0);
-      if (Writer.Chain)
-        AddFirstDeclFromEachModule(DAsT, /*IncludeLocal*/false);
+      if (Writer.Chain) {
+        // Namespace can have many redeclaration in many TU.
+        // We try to avoid touching every redeclaration to try to
+        // reduce the dependencies as much as possible.
+        if (!isa<NamespaceDecl>(DAsT))
+          AddFirstDeclFromEachModule(DAsT, /*IncludeLocal=*/ false);
+        else
+          Record.AddDeclRef(/*Decl=*/nullptr);
+      }
       // This is the number of imported first declarations + 1.
       Record[I] = Record.size() - I;
 
@@ -2265,8 +2272,10 @@ void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) {
     //
     // FIXME: This is not correct; when we reach an imported declaration we
     // won't emit its previous declaration.
-    (void)Writer.GetDeclRef(D->getPreviousDecl());
-    (void)Writer.GetDeclRef(MostRecent);
+    if (D->getPreviousDecl() && !D->getPreviousDecl()->isFromASTFile())
+      (void)Writer.GetDeclRef(D->getPreviousDecl());
+    if (!MostRecent->isFromASTFile())
+      (void)Writer.GetDeclRef(MostRecent);
   } else {
     // We use the sentinel value 0 to indicate an only declaration.
     Record.push_back(0);

clang/test/Modules/no-transitive-change-namespace.cppm

@@ -0,0 +1,44 @@
+// Test that adding a new decl in a module which originally only contained a namespace
+// won't break the dependency.
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/Root.cppm -o %t/Root.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/N.cppm -o %t/N.pcm \
+// RUN:     -fmodule-file=Root=%t/Root.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/N.v1.cppm -o %t/N.v1.pcm \
+// RUN:     -fmodule-file=Root=%t/Root.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/M.cppm -o %t/M.pcm \
+// RUN:     -fmodule-file=N=%t/N.pcm -fmodule-file=Root=%t/Root.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/M.cppm -o %t/M.v1.pcm \
+// RUN:     -fmodule-file=N=%t/N.v1.pcm -fmodule-file=Root=%t/Root.pcm
+//
+// RUN: diff %t/M.pcm %t/M.v1.pcm &> /dev/null
+
+//--- Root.cppm
+export module Root;
+export namespace N {
+
+}
+
+//--- N.cppm
+export module N;
+import Root;
+export namespace N {
+
+}
+
+//--- N.v1.cppm
+export module N;
+import Root;
+export namespace N {
+struct NN {};
+}
+
+//--- M.cppm
+export module M;
+import N;
+export namespace N {
+struct MM {};
+}

clang/test/Modules/no-transitive-decl-change-4.cppm

@@ -27,7 +27,7 @@
 // RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/B.cppm -o %t/B.v1.pcm \
 // RUN:     -fprebuilt-module-path=%t -fmodule-file=AWrapper=%t/AWrapper.v1.pcm -fmodule-file=A=%t/A.v1.pcm
 //
-// RUN: not diff %t/B.pcm %t/B.v1.pcm &> /dev/null
+// RUN: diff %t/B.pcm %t/B.v1.pcm &> /dev/null
 
 //--- T.cppm
 export module T;

clang/test/Modules/pr184957.cppm

@@ -0,0 +1,216 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+// RUN: mkdir %t/tmp
+//
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std %t/wrap.std.tt.cppm -emit-reduced-module-interface -o %t/wrap.std.tt.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std %t/wrap.std.vec.cppm -emit-reduced-module-interface -o %t/wrap.std.vec.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std %t/not_std.cppm -emit-reduced-module-interface -o %t/not_std.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std %t/wrap.std.tt2.cppm -emit-reduced-module-interface -o %t/wrap.std.tt2.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std -fmodule-file=wrap.std.vec=%t/wrap.std.vec.pcm %t/wrap.std.vec.reexport.cppm -emit-reduced-module-interface -o %t/wrap.std.vec.reexport.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std -fmodule-file=wrap.std.tt=%t/wrap.std.tt.pcm -fmodule-file=wrap.std.vec=%t/wrap.std.vec.pcm -fmodule-file=wrap.std.vec.reexport=%t/wrap.std.vec.reexport.pcm -fmodule-file=wrap.std.tt2=%t/wrap.std.tt2.pcm -fmodule-file=not_std=%t/not_std.pcm %t/k.repro_dep.cppm -emit-reduced-module-interface -o %t/k.repro_dep.pcm
+// RUN: %clang_cc1 -std=c++23 -nostdinc++ -I %t/std -fmodule-file=wrap.std.tt=%t/wrap.std.tt.pcm -fmodule-file=wrap.std.vec=%t/wrap.std.vec.pcm -fmodule-file=wrap.std.tt2=%t/wrap.std.tt2.pcm -fmodule-file=wrap.std.vec.reexport=%t/wrap.std.vec.reexport.pcm -fmodule-file=not_std=%t/not_std.pcm -fmodule-file=k.repro_dep=%t/k.repro_dep.pcm %t/k.repro.cxx -fsyntax-only -verify
+
+//--- std/allocator.h
+#ifndef _LIBCPP___MEMORY_ALLOCATOR_H
+#define _LIBCPP___MEMORY_ALLOCATOR_H
+
+#include <size_t.h>
+
+namespace std {
+
+enum align_val_t { __zero = 0, __max = (size_t)-1 };
+
+template <class _Tp>
+inline _Tp*
+__libcpp_allocate(size_t __n, [[__maybe_unused__]] size_t __align = 8) {
+  size_t __size = static_cast<size_t>(__n) * sizeof(_Tp);
+  return static_cast<_Tp*>(__builtin_operator_new(__size, static_cast<align_val_t>(__align)));
+}
+
+template <class _Tp>
+class allocator
+{
+public:
+  typedef _Tp value_type;
+
+  [[__nodiscard__]] _Tp* allocate(size_t __n) {
+    if (__builtin_is_constant_evaluated()) {
+      return static_cast<_Tp*>(::operator new(__n * sizeof(_Tp)));
+    } else {
+      return std::__libcpp_allocate<_Tp>(size_t(__n));
+    }
+  }
+};
+
+template <class _Tp, class _Up>
+inline bool
+operator==(const allocator<_Tp>&, const allocator<_Up>&) noexcept {
+  return true;
+}
+
+}
+
+#endif // _LIBCPP___MEMORY_ALLOCATOR_H
+
+//--- std/sys_wchar.h
+#ifndef _WCHAR_H
+#define _WCHAR_H 1
+
+#ifndef __FILE_defined
+  #define __FILE_defined 1
+
+  struct _IO_FILE;
+
+  typedef struct _IO_FILE FILE;
+#endif
+
+#endif /* wchar.h  */
+
+//--- std/char_traits.h
+#ifndef _LIBCPP___STRING_CHAR_TRAITS_H
+#define _LIBCPP___STRING_CHAR_TRAITS_H
+
+namespace std {
+
+template <class _Tp>
+inline size_t constexpr __constexpr_strlen(const _Tp* __str) noexcept {
+  if (__builtin_is_constant_evaluated()) {
+    size_t __i = 0;
+    for (; __str[__i] != '\0'; ++__i)
+      ;
+    return __i;
+  }
+  return 0;
+}
+
+template <class _CharT>
+struct char_traits;
+
+template <>
+struct char_traits<char> {
+  using char_type  = char;
+  
+  [[__nodiscard__]] static inline size_t constexpr
+  length(const char_type* __s) noexcept {
+    return std::__constexpr_strlen(__s);
+  }
+};
+
+}
+
+#endif // _LIBCPP___STRING_CHAR_TRAITS_H
+
+//--- std/type_traits
+#ifndef _LIBCPP_TYPE_TRAITS
+#define _LIBCPP_TYPE_TRAITS
+
+namespace std
+{}
+
+#endif // _LIBCPP_TYPE_TRAITS
+
+//--- std/ptrdiff_t.h
+#ifndef _LIBCPP___CSTDDEF_PTRDIFF_T_H
+#define _LIBCPP___CSTDDEF_PTRDIFF_T_H
+
+namespace std {
+
+using ptrdiff_t = decltype(static_cast<int*>(nullptr) - static_cast<int*>(nullptr));
+
+}
+
+#endif // _LIBCPP___CSTDDEF_PTRDIFF_T_H
+
+//--- std/size_t.h
+#ifndef _LIBCPP___CSTDDEF_SIZE_T_H
+#define _LIBCPP___CSTDDEF_SIZE_T_H
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+}
+
+#endif // _LIBCPP___CSTDDEF_SIZE_T_H
+
+//--- wrap.std.tt.cppm
+module;
+
+#include <type_traits>
+
+export module wrap.std.tt;
+
+//--- wrap.std.vec.cppm
+module;
+
+#include <allocator.h>
+#include <sys_wchar.h>
+
+export module wrap.std.vec;
+
+//--- not_std.cppm
+module;
+
+#include <allocator.h>
+
+
+export module not_std;
+
+namespace std {
+  template <class _Tp, class _Allocator>
+  class __split_buffer {
+  public:
+    __split_buffer() {
+      _Allocator a;
+      (void)a.allocate(1);
+    }
+  };
+}
+
+export namespace std {
+  template <typename T>
+  using problem = std::__split_buffer<T, std::allocator<T>>;
+}
+
+export using ::operator new;
+
+//--- wrap.std.tt2.cppm
+module;
+
+#include <type_traits>
+
+export module wrap.std.tt2;
+
+//--- wrap.std.vec.reexport.cppm
+export module wrap.std.vec.reexport;
+
+export import wrap.std.vec;
+
+//--- k.repro_dep.cppm
+module;
+
+#include <allocator.h>
+#include <sys_wchar.h>
+#include <char_traits.h>
+
+export module k.repro_dep;
+
+import wrap.std.tt;
+import wrap.std.vec.reexport;
+import wrap.std.vec;
+import wrap.std.tt2;
+
+import not_std;
+
+using XYZ = std::char_traits<char>;
+
+//--- k.repro.cxx
+// expected-no-diagnostics
+import not_std;
+import k.repro_dep;
+
+auto f() -> void
+{
+  std::problem< int > x;
+}