[C++20] [Modules] Implementing Eliding Unreachable Decls of GMF in ASTWriter #76930
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Chuanqi Xu (ChuanqiXu9) ChangesThis was a patch to try to implement eliding unreachable decls in GMF in ASTWriter. It was developed a half year ago and I just rebased it but I did't fix the failing test. It ran well. The core idea of the patch is that we can implement the idea reachable in ASTWriter naturally. The secret is that we skip writing GMF initially (generally we will write decls from the top to the bottom) and we start to write the declarations from module purview. Then we will only write the declarations in GMF if it is mentioned during the writing process. So the unreachable decls won't be written natually. The experience in implementing this patch is pretty smooth and the tests from the spec can be passed. I felt this should be the natural way to implement this feature. The only one and big problem is that we didn't implement the formal semantics in the spec in this way : | Patch is 32.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76930.diff 26 Files Affected:
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 10dcbdb262d84e..523b930d59645a 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -233,9 +233,12 @@ class alignas(8) Decl {
/// This declaration has an owning module, but is only visible to
/// lookups that occur within that module.
- /// The discarded declarations in global module fragment belongs
- /// to this group too.
- ModulePrivate
+ ModulePrivate,
+
+ /// This declaration is part of a Global Module Fragment, it is permitted
+ /// to discard it and therefore it is not reachable or visible to importers
+ /// of the named module of which the GMF is part.
+ ModuleDiscardable
};
protected:
@@ -658,9 +661,10 @@ class alignas(8) Decl {
/// Whether this declaration comes from another module unit.
bool isInAnotherModuleUnit() const;
- /// FIXME: Implement discarding declarations actually in global module
- /// fragment. See [module.global.frag]p3,4 for details.
- bool isDiscardedInGlobalModuleFragment() const { return false; }
+ /// See [module.global.frag]p3,4 for details.
+ bool isDiscardedInGlobalModuleFragment() const {
+ return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleDiscardable;
+ }
/// Return true if this declaration has an attribute which acts as
/// definition of the entity, such as 'alias' or 'ifunc'.
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 21abc346cf17ac..97ba8147bdfbb5 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -178,6 +178,7 @@ LANGOPT(BuiltinHeadersInSystemModules, 1, 0, "builtin headers belong to system m
BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
"compiling a module interface")
BENIGN_LANGOPT(CompilingPCH, 1, 0, "building a pch")
+LANGOPT(DiscardGMFDecls , 1, 1, "Discard unreachable decls in GMF")
BENIGN_LANGOPT(BuildingPCHWithObjectFile, 1, 0, "building a pch which has a corresponding object file")
BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "instantiate templates while building a PCH")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 2b93ddf033499c..127434550276e0 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2970,6 +2970,13 @@ defm prebuilt_implicit_modules : BoolFOption<"prebuilt-implicit-modules",
PosFlag<SetTrue, [], [ClangOption], "Look up implicit modules in the prebuilt module path">,
NegFlag<SetFalse>, BothFlags<[], [ClangOption, CC1Option]>>;
+defm discarding_gmf_decls : BoolFOption<"discarding-gmf-decls",
+ LangOpts<"DiscardGMFDecls">, DefaultTrue,
+ PosFlag<SetTrue>,
+ NegFlag<SetFalse, [], [ClangOption],
+ "Disable to discard unreachable decls in global module fragment">,
+ BothFlags<[], [ClangOption, CC1Option]>>;
+
def fmodule_output_EQ : Joined<["-"], "fmodule-output=">,
Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index de69f99003d827..fdd142ac7e20da 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -467,6 +467,22 @@ class ASTWriter : public ASTDeserializationListener,
std::vector<SourceRange> NonAffectingRanges;
std::vector<SourceLocation::UIntTy> NonAffectingOffsetAdjustments;
+ /// Mark ModuleDiscardable Decl D and its file scope top level declaration D
+ /// as reachable. This is a no-op if D is not ModuleDiscardable. We'll mark
+ /// the file scope top level declaration D as reachable too. Otherwise, it is
+ /// problematic if some parts of a decl is discarded in some TU and these
+ /// parts are not discarded in other TUs. This is an ODR violation. So if a
+ /// sub-decl is reachable, the top level decl and all of its children should
+ /// be reachable too.
+ void MarkDeclReachable(const Decl *D);
+ /// A helper to IsDeclModuleDiscardable. There are special declarations which
+ /// may not be referenced directly. But they can't be discarded if their
+ /// correspond decls are reachable. e.g., the deduction guides decls.
+ bool IsSpecialDeclNotDiscardable(Decl *D);
+ /// Callbacks to mark special decls as reachable once their corresponding
+ /// decls become reachable.
+ llvm::DenseMap<Decl *, llvm::SmallVector<Decl *, 8>> ReachableMarkerCallbacks;
+
/// Collects input files that didn't affect compilation of the current module,
/// and initializes data structures necessary for leaving those files out
/// during \c SourceManager serialization.
@@ -792,6 +808,14 @@ class ASTWriter : public ASTDeserializationListener,
return WritingModule && WritingModule->isNamedModule();
}
+ /// Whether or not D is module discardable. Besides the case that D is marked
+ /// not module discardable explicitly, `IsDeclModuleDiscardable` will return
+ /// false if:
+ /// - The file scope top level declaration of D is not module discardable.
+ /// - D is a deduction guide for another template declaration TD and TD is not
+ /// module discardable.
+ bool IsDeclModuleDiscardable(const Decl *D);
+
private:
// ASTDeserializationListener implementation
void ReaderInitialized(ASTReader *Reader) override;
diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp
index ed7f626971f345..ac68d0e5106d7a 100644
--- a/clang/lib/Sema/SemaModule.cpp
+++ b/clang/lib/Sema/SemaModule.cpp
@@ -88,7 +88,11 @@ Sema::ActOnGlobalModuleFragmentDecl(SourceLocation ModuleLoc) {
// within the module unit.
//
// So the declations in the global module shouldn't be visible by default.
- TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
+ if (getLangOpts().DiscardGMFDecls)
+ TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModuleDiscardable);
+ else
+ TU->setModuleOwnershipKind(
+ Decl::ModuleOwnershipKind::ReachableWhenImported);
TU->setLocalOwningModule(GlobalModule);
// FIXME: Consider creating an explicit representation of this declaration.
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 547eb77930b4ee..a76740d0898a8f 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -656,6 +656,9 @@ void ASTDeclReader::VisitDecl(Decl *D) {
case Decl::ModuleOwnershipKind::ReachableWhenImported:
case Decl::ModuleOwnershipKind::ModulePrivate:
break;
+
+ case Decl::ModuleOwnershipKind::ModuleDiscardable:
+ llvm_unreachable("We should never read module discardable decls");
}
D->setModuleOwnershipKind(ModuleOwnership);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 78939bfd533ffa..4823cbd5dd4f21 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -2994,7 +2994,8 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
// Emit the initializers, if any.
RecordData Inits;
for (Decl *D : Context->getModuleInitializers(Mod))
- Inits.push_back(GetDeclRef(D));
+ if (!IsDeclModuleDiscardable(D))
+ Inits.push_back(GetDeclRef(D));
if (!Inits.empty())
Stream.EmitRecord(SUBMODULE_INITIALIZERS, Inits);
@@ -3171,6 +3172,13 @@ uint64_t ASTWriter::WriteDeclContextLexicalBlock(ASTContext &Context,
uint64_t Offset = Stream.GetCurrentBitNo();
SmallVector<uint32_t, 128> KindDeclPairs;
for (const auto *D : DC->decls()) {
+ if (IsDeclModuleDiscardable(D)) {
+ if (DC->isFileContext())
+ continue;
+ else
+ MarkDeclReachable(D);
+ }
+
KindDeclPairs.push_back(D->getKind());
KindDeclPairs.push_back(GetDeclRef(D));
}
@@ -3819,9 +3827,12 @@ class ASTDeclContextNameLookupTrait {
template<typename Coll>
data_type getData(const Coll &Decls) {
unsigned Start = DeclIDs.size();
- for (NamedDecl *D : Decls) {
- DeclIDs.push_back(
- Writer.GetDeclRef(getDeclForLocalLookup(Writer.getLangOpts(), D)));
+ for (NamedDecl *ND : Decls) {
+ auto *D = getDeclForLocalLookup(Writer.getLangOpts(), ND);
+ if (Writer.IsDeclModuleDiscardable(D))
+ continue;
+
+ DeclIDs.push_back(Writer.GetDeclRef(D));
}
return std::make_pair(Start, DeclIDs.size());
}
@@ -3976,6 +3987,15 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
isLookupResultEntirelyExternal(Result, DC))
continue;
+ if (!DC->isFileContext() && !DoneWritingDeclsAndTypes) {
+ for (auto *D : Result.getLookupResult())
+ if (IsDeclModuleDiscardable(D))
+ MarkDeclReachable(D);
+ } else if (llvm::all_of(Result.getLookupResult(), [this](NamedDecl *D) {
+ return IsDeclModuleDiscardable(D);
+ }))
+ continue;
+
// We also skip empty results. If any of the results could be external and
// the currently available results are empty, then all of the results are
// external and we skip it above. So the only way we get here with an empty
@@ -4165,7 +4185,7 @@ uint64_t ASTWriter::WriteDeclContextVisibleBlock(ASTContext &Context,
}
for (NamedDecl *ND : Result)
- if (!ND->isFromASTFile())
+ if (!ND->isFromASTFile() && !IsDeclModuleDiscardable(ND))
GetDeclRef(ND);
}
@@ -4903,7 +4923,7 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
const TranslationUnitDecl *TU = Context.getTranslationUnitDecl();
SmallVector<uint32_t, 128> NewGlobalKindDeclPairs;
for (const auto *D : TU->noload_decls()) {
- if (!D->isFromASTFile()) {
+ if (!D->isFromASTFile() && !IsDeclModuleDiscardable(D)) {
NewGlobalKindDeclPairs.push_back(D->getKind());
NewGlobalKindDeclPairs.push_back(GetDeclRef(D));
}
@@ -4955,9 +4975,9 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
// Make sure visible decls, added to DeclContexts previously loaded from
// an AST file, are registered for serialization. Likewise for template
// specializations added to imported templates.
- for (const auto *I : DeclsToEmitEvenIfUnreferenced) {
- GetDeclRef(I);
- }
+ for (const auto *I : DeclsToEmitEvenIfUnreferenced)
+ if (!IsDeclModuleDiscardable(I))
+ GetDeclRef(I);
// Make sure all decls associated with an identifier are registered for
// serialization, if we're storing decls with identifiers.
@@ -5280,6 +5300,7 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
case UPD_CXX_ADDED_TEMPLATE_SPECIALIZATION:
case UPD_CXX_ADDED_ANONYMOUS_NAMESPACE:
assert(Update.getDecl() && "no decl to add?");
+ MarkDeclReachable(Update.getDecl());
Record.push_back(GetDeclRef(Update.getDecl()));
break;
@@ -5419,6 +5440,7 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
Record.AddVarDeclInit(VD);
}
+ MarkDeclReachable(D);
OffsetsRecord.push_back(GetDeclRef(D));
OffsetsRecord.push_back(Record.Emit(DECL_UPDATES));
}
@@ -5679,7 +5701,125 @@ TypeID ASTWriter::getTypeID(QualType T) const {
});
}
+bool ASTWriter::IsSpecialDeclNotDiscardable(Decl *D) {
+ assert(D->isDiscardedInGlobalModuleFragment());
+
+ /// Currently, the only special decl is the deduction guide.
+ if (auto *ND = dyn_cast<NamedDecl>(D)) {
+ DeclarationName Name = ND->getDeclName();
+ if (TemplateDecl *TD = Name.getCXXDeductionGuideTemplate()) {
+ if (!IsDeclModuleDiscardable(TD)) {
+ MarkDeclReachable(D);
+ return true;
+ }
+
+ ReachableMarkerCallbacks[TD].push_back(D);
+ }
+ }
+
+ // FIXME:
+ //
+ // There is a wide used pattern in libstdc++:
+ //
+ // namespace std
+ // {
+ // inline namespace __cxx11 __attribute__((__abi_tag__ ("cxx11"))) { }
+ // }
+ // namespace __gnu_cxx
+ // {
+ // inline namespace __cxx11 __attribute__((__abi_tag__ ("cxx11"))) { }
+ // }
+ // ...
+ // namespace std {
+ // some declarations for STL.
+ // ...
+ // namespace __cxx11 {
+ // some declarations for STL.
+ // }
+ // }
+ //
+ // Then in a real project, we observed false-positive ODR violations
+ // since some module units discard `__gnu_cxx::__cxx11` namespace while
+ // other module units don't discard `__gnu_cxx::__cxx11` namespace.
+ //
+ // This may imply that the ODR checking process is context sensitive.
+ // That said, the same type in different module units can be considered to be
+ // different if some module units discard the unused `__gnu_cxx::__cxx11` namespace
+ // while other module units don't. This is incorrect.
+ //
+ // This is a workaround to make things happen but we indeed to fix the ODR checking
+ // process indeed.
+ if (auto *ND = dyn_cast<NamedDecl>(D);
+ ND && ND->getAttr<AbiTagAttr>()) {
+ MarkDeclReachable(D);
+ return true;
+ }
+
+ return false;
+}
+
+bool ASTWriter::IsDeclModuleDiscardable(const Decl *ConstD) {
+ Decl *D = const_cast<Decl *>(ConstD);
+
+ if (!D->isDiscardedInGlobalModuleFragment())
+ return false;
+
+ // The Translation Unit should never be module discardable.
+ if (!D->getDeclContext()) {
+ assert(isa<TranslationUnitDecl>(D));
+ return false;
+ }
+
+ if (IsSpecialDeclNotDiscardable(D))
+ return false;
+
+ const DeclContext *DC = D->getNonTransparentDeclContext();
+ while (DC && DC->getParent() &&
+ !DC->getParent()->getNonTransparentContext()->isFileContext())
+ DC = DC->getParent()->getNonTransparentContext();
+
+ assert(DC && "Why is the decl not covered by file context?");
+ if (!DC->isFileContext() && !cast<Decl>(DC)->isDiscardedInGlobalModuleFragment()) {
+ MarkDeclReachable(D);
+ return false;
+ }
+
+ return true;
+}
+
+void ASTWriter::MarkDeclReachable(const Decl *ConstD) {
+ Decl *D = const_cast<Decl *>(ConstD);
+
+ if (!D || !D->isDiscardedInGlobalModuleFragment())
+ return;
+
+ D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
+ if (D->getNonTransparentDeclContext()->isFileContext()) {
+ // Update the decl contexts so that we can still find the decl with name
+ // lookup.
+ UpdatedDeclContexts.insert(D->getNonTransparentDeclContext());
+ }
+
+ auto Iter = ReachableMarkerCallbacks.find(D);
+ if (Iter != ReachableMarkerCallbacks.end()) {
+ for (Decl *ToBeMarked : Iter->second) {
+ MarkDeclReachable(ToBeMarked);
+ GetDeclRef(ToBeMarked);
+ }
+ ReachableMarkerCallbacks.erase(D);
+ }
+
+ DeclContext *DC = D->getNonTransparentDeclContext();
+ while (DC && DC->getParent() &&
+ !DC->getParent()->getNonTransparentContext()->isFileContext())
+ DC = DC->getParent()->getNonTransparentContext();
+
+ if (DC && !DC->isFileContext())
+ MarkDeclReachable(cast<Decl>(DC));
+}
+
void ASTWriter::AddDeclRef(const Decl *D, RecordDataImpl &Record) {
+ MarkDeclReachable(D);
Record.push_back(GetDeclRef(D));
}
@@ -5695,6 +5835,8 @@ DeclID ASTWriter::GetDeclRef(const Decl *D) {
if (D->isFromASTFile())
return D->getGlobalID();
+ assert(!D->isDiscardedInGlobalModuleFragment() && "We shouldn't write discarded decl.\n");
+
assert(!(reinterpret_cast<uintptr_t>(D) & 0x01) && "Invalid decl pointer");
DeclID &ID = DeclIDs[D];
if (ID == 0) {
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 9e3299f0491848..cd605cfbadc11b 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1987,6 +1987,8 @@ void ASTDeclWriter::VisitRedeclarable(Redeclarable<T> *D) {
//
// FIXME: This is not correct; when we reach an imported declaration we
// won't emit its previous declaration.
+ Writer.MarkDeclReachable(D->getPreviousDecl());
+ Writer.MarkDeclReachable(MostRecent);
(void)Writer.GetDeclRef(D->getPreviousDecl());
(void)Writer.GetDeclRef(MostRecent);
} else {
diff --git a/clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp b/clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
index d69db40062dae9..5e48300244655e 100644
--- a/clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
+++ b/clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
@@ -30,7 +30,6 @@ module;
void test_early() {
in_header = 1; // expected-error {{use of undeclared identifier 'in_header'}}
- // expected-note@* {{not visible}}
global_module_fragment = 1; // expected-error {{use of undeclared identifier 'global_module_fragment'}}
@@ -53,10 +52,9 @@ import A;
#endif
void test_late() {
- in_header = 1; // expected-error {{missing '#include "foo.h"'; 'in_header' must be declared before it is used}}
- // expected-note@* {{not visible}}
+ in_header = 1; // expected-error {{use of undeclared identifier 'in_header'}}
- global_module_fragment = 1; // expected-error {{missing '#include'; 'global_module_fragment' must be declared before it is used}}
+ global_module_fragment = 1; // expected-error {{use of undeclared identifier 'global_module_fragment'}}
exported = 1;
diff --git a/clang/test/CXX/module/module.glob.frag/cxx20-10-4-ex2.cppm b/clang/test/CXX/module/module.glob.frag/cxx20-10-4-ex2.cppm
new file mode 100644
index 00000000000000..592f926e391e96
--- /dev/null
+++ b/clang/test/CXX/module/module.glob.frag/cxx20-10-4-ex2.cppm
@@ -0,0 +1,72 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 %t/std-10-4-ex2-interface.cppm -emit-module-interface \
+// RUN: -o %t/M.pcm -Wno-unused-value
+// RUN: %clang_cc1 -std=c++20 %t/std-10-4-ex2-implementation.cpp -fmodule-file=M=%t/M.pcm \
+// RUN: -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/std-10-4-ex2-interface.cppm -emit-module-interface \
+// RUN: -o %t/M.pcm -Wno-unused-value -fno-discarding-gmf-decls
+// RUN: %clang_cc1 -std=c++20 %t/std-10-4-ex2-implementation.cpp -fmodule-file=M=%t/M.pcm \
+// RUN: -fsyntax-only -verify -fno-discarding-gmf-decls -DNO_DISCARD
+
+//--- std-10-4-ex2.h
+
+namespace N {
+struct X {};
+int d();
+int e();
+inline int f(X, int = d()) { return e(); }
+int g(X);
+int h(X);
+} // namespace N
+
+//--- std-10-4-ex2-interface.cppm
+
+module;
+
+#include "std-10-4-ex2.h"
+
+export module M;
+
+template <typename T> int use_f() {
+ N::X x; // N::X, N, and :: are decl-reachable from use_f
+ return f(x, 123); // N::f is decl-reachable from use_f,
+ // N::e is indirectly decl-reachable from use_f
+ // because it is decl-reachable from N::f, and
+ // N::d is decl-reachable from use_f
+ // because it is decl-reachable from N::f
+ // even though it is not used in this call
+}
+
+template <typename T> int use_g() {
+ N::X x; // N::X, N, and :: are decl-reachable from use_g
+ return g((T(), x)); // N::g is not decl-reachable from use_g
+}
+
+template <typename T> int use_h() {
+ N::X x; // N::X, N, and :: are decl-reachable from use_h
+ return h((T(), x)); // N::h is not decl-reachable from use_h, but
+ // N::h is decl-reachable from use_h<int>
+}
+
+int k = use_h<int>();
+// use_h<int> is decl-reachable from k, so
+// N::h is decl-reachable from k
+
+//--- std-10-4-ex2-implementation.cpp
+#ifdef NO_DISCARD
+// expected-no-diagnostics
+#endif
+
+module M;
+
+int a = use_f<int>();
+int b = use_g<int>();
+#ifndef NO_DISCARD
+// expected-error@std-10-4-ex2-interface.cppm:20 {{use of undeclared identifier 'g'}}
+// expected-note@-3 {{in instantiation of function template specialization 'use_g<int>' requested here}}
+#endif
+int c = use_h<int>();
diff --git a/clang/test/CXX/module/module.import/p2.cpp b/clang/test/CXX/module/module.import/p2.cpp
index ef6006811e7763..21068a0fa30d5d 100644
--- a/clang/test/CXX/module/module.import/p2.cpp
+++ b/clang/test/CXX/module/module.import/p2.cpp
@@ -67,13 +67,14 @@ void test() {
module;
class A{}...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 7a3b0cbb143d02b70b2bfae5cd40e9867c124748 b9a03912276d25ff819a755bef4ee72d64ce1480 -- clang/test/CXX/module/module.glob.frag/cxx20-10-4-ex2.cppm clang/test/Modules/abi-tag.cppm clang/include/clang/AST/DeclBase.h clang/include/clang/Serialization/ASTWriter.h clang/lib/Sema/SemaModule.cpp clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp clang/test/CXX/module/module.import/p2.cpp clang/test/CodeGenCXX/module-intializer-pmf.cpp clang/test/CodeGenCXX/module-intializer.cpp clang/test/Modules/concept.cppm clang/test/Modules/explicitly-specialized-template.cpp clang/test/Modules/inconsistent-deduction-guide-linkage.cppm clang/test/Modules/named-modules-adl-2.cppm clang/test/Modules/named-modules-adl.cppm clang/test/Modules/polluted-operator.cppm clang/test/Modules/pr58716.cppm clang/test/Modules/pr60775.cppm clang/test/Modules/pr62589.cppm clang/test/Modules/preferred_name.cppm clang/test/Modules/redundant-template-default-arg3.cpp clang/test/Modules/template-function-specialization.cppView the diff from clang-format here.diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 4823cbd5dd..b981a8e5bd 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3992,9 +3992,9 @@ ASTWriter::GenerateNameLookupTable(const DeclContext *ConstDC,
if (IsDeclModuleDiscardable(D))
MarkDeclReachable(D);
} else if (llvm::all_of(Result.getLookupResult(), [this](NamedDecl *D) {
- return IsDeclModuleDiscardable(D);
- }))
- continue;
+ return IsDeclModuleDiscardable(D);
+ }))
+ continue;
// We also skip empty results. If any of the results could be external and
// the currently available results are empty, then all of the results are
@@ -5744,13 +5744,12 @@ bool ASTWriter::IsSpecialDeclNotDiscardable(Decl *D) {
//
// This may imply that the ODR checking process is context sensitive.
// That said, the same type in different module units can be considered to be
- // different if some module units discard the unused `__gnu_cxx::__cxx11` namespace
- // while other module units don't. This is incorrect.
+ // different if some module units discard the unused `__gnu_cxx::__cxx11`
+ // namespace while other module units don't. This is incorrect.
//
- // This is a workaround to make things happen but we indeed to fix the ODR checking
- // process indeed.
- if (auto *ND = dyn_cast<NamedDecl>(D);
- ND && ND->getAttr<AbiTagAttr>()) {
+ // This is a workaround to make things happen but we indeed to fix the ODR
+ // checking process indeed.
+ if (auto *ND = dyn_cast<NamedDecl>(D); ND && ND->getAttr<AbiTagAttr>()) {
MarkDeclReachable(D);
return true;
}
@@ -5779,7 +5778,8 @@ bool ASTWriter::IsDeclModuleDiscardable(const Decl *ConstD) {
DC = DC->getParent()->getNonTransparentContext();
assert(DC && "Why is the decl not covered by file context?");
- if (!DC->isFileContext() && !cast<Decl>(DC)->isDiscardedInGlobalModuleFragment()) {
+ if (!DC->isFileContext() &&
+ !cast<Decl>(DC)->isDiscardedInGlobalModuleFragment()) {
MarkDeclReachable(D);
return false;
}
@@ -5835,7 +5835,8 @@ DeclID ASTWriter::GetDeclRef(const Decl *D) {
if (D->isFromASTFile())
return D->getGlobalID();
- assert(!D->isDiscardedInGlobalModuleFragment() && "We shouldn't write discarded decl.\n");
+ assert(!D->isDiscardedInGlobalModuleFragment() &&
+ "We shouldn't write discarded decl.\n");
assert(!(reinterpret_cast<uintptr_t>(D) & 0x01) && "Invalid decl pointer");
DeclID &ID = DeclIDs[D];
|
This was referenced Mar 20, 2024
|
I took another review on this patch and it shows the patch itself is not so correct. The idea of the patch is to skip GMF at first and start to write the module from the module purview directly. Then everything unused in GMF is not used. This seems fine. However, in the module purview, if we used something from a namespace of the GMF, we would everything in the namespace back. This is terrible. So in this patch, when we visit decl context, we skipped adding unreached declarations. However, this is the problem. Since an unreached declaration during the process of writing become be reachable in the end of the writing. However, we've skipped it. To solve this, either we need to write the declaration context lazily or perform a walk ahead of time. But clearly, we can't continue with the current patch. I'll bring a new one after I figured it out. |
ChuanqiXu9
added a commit
that referenced
this pull request
Apr 1, 2024
…type decl offsets This patch reorder the lexical block for the translation unit, visible update block for the TU and the viisble upaete block for the extern C context after the type decl offsets block. This should be a NFC patch. This is helpful for later optimizations for eliding unreachable declarations in the global module fragment. See the comments in #76930. Simply, if we want to get the reachable sets of declaratins during the writing process, we need to write the file-level context later than the process of writing declarations (which is the main process to determine the reachable set).
ChuanqiXu9
added a commit
to ChuanqiXu9/llvm-project
that referenced
this pull request
Apr 11, 2024
…ed BMI Following of llvm#76930 This follows the idea of "only writes what we writes", which I think is the most natural and efficient way to implement this optimization. We start writing the BMI from the first declaration in module purview instead of the global module fragment, so that everything in the GMF untouched won't be written in the BMI naturally. The exception is, as I said in llvm#76930, when we write a declaration we need to write its decl context, and when we write the decl context, we need to write everything from it. So when we see `std::vector`, we basically need to write everything under namespace std. This violates our intention. To fix this, this patch delays the writing of namespace in the GMF. From my local measurement, the size of the BMI decrease to 90M from 112M for a local modules build. I think this is significant. This feature will be covered under the experimental reduced BMI so that it won't affect any existing users. So I'd like to land this when the CI gets green. Documents will be added seperately.
ChuanqiXu9
added a commit
that referenced
this pull request
Apr 12, 2024
…d BMI (#88359) Following of #76930 This follows the idea of "only writes what we writes", which I think is the most natural and efficient way to implement this optimization. We start writing the BMI from the first declaration in module purview instead of the global module fragment, so that everything in the GMF untouched won't be written in the BMI naturally. The exception is, as I said in #76930, when we write a declaration we need to write its decl context, and when we write the decl context, we need to write everything from it. So when we see `std::vector`, we basically need to write everything under namespace std. This violates our intention. To fix this, this patch delays the writing of namespace in the GMF. From my local measurement, the size of the BMI decrease to 90M from 112M for a local modules build. I think this is significant. This feature will be covered under the experimental reduced BMI so that it won't affect any existing users. So I'd like to land this when the CI gets green. Documents will be added seperately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was a patch to try to implement eliding unreachable decls in GMF in ASTWriter. It was developed a half year ago and I just rebased it but I did't fix the failing test. It ran well.
The core idea of the patch is that we can implement the idea reachable in ASTWriter naturally.
The secret is that we skip writing GMF initially (generally we will write decls from the top to the bottom) and we start to write the declarations from module purview. Then we will only write the declarations in GMF if it is mentioned during the writing process. So the unreachable decls won't be written natually.
The experience in implementing this patch is pretty smooth and the tests from the spec can be passed. I felt this should be the natural way to implement this feature.
The only one and big problem is that we didn't implement the formal semantics in the spec in this way : |