[clang] c184b94 - [C++20] [Modules] Write ODRHash for decls in GMF
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 17 20:44:15 PDT 2024
Author: Chuanqi Xu
Date: 2024-07-18T11:42:23+08:00
New Revision: c184b94ff6546c8ba8ac54b5127189427567978f
URL: https://github.com/llvm/llvm-project/commit/c184b94ff6546c8ba8ac54b5127189427567978f
DIFF: https://github.com/llvm/llvm-project/commit/c184b94ff6546c8ba8ac54b5127189427567978f.diff
LOG: [C++20] [Modules] Write ODRHash for decls in GMF
Previously, we skipped calculating ODRHash for decls in GMF when writing
them to .pcm files as an optimization. But actually, it is not
true that this will be a pure optimization. Whether or not it is
beneficial depends on the use cases. For example, if we're writing a
function `a` in module and there are 10 consumers of `a` in other TUs,
then the other TUs will pay for the cost to calculate the ODR hash for
`a` ten times. Then this optimization doesn't work. However, if all the
consumers of the module didn't touch `a`, then we can save the cost to
calculate the ODR hash of `a` for 1 times.
And the assumption to make it was: generally, the consumers of a module
may only consume a small part of the imported module. This is the reason
why we tried to load declarations, types and identifiers lazily. Then it
looks good to do the similar thing for calculating ODR hashs.
It works fine for a long time, until we started to look into the support
of modules in clangd. Then we meet multiple issue reports complaining
we're calculating ODR hash in the wrong place. To workaround these issue
reports, I decided to always write the ODRhash for decls in GMF. In my
local test, I only observed less than 1% compile time regression after
doing this. So it should be fine.
Added:
Modified:
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
Removed:
################################################################################
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 76032aa836b50..3de9d327f1b3b 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -794,15 +794,12 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
BitsUnpacker EnumDeclBits(Record.readInt());
ED->setNumPositiveBits(EnumDeclBits.getNextBits(/*Width=*/8));
ED->setNumNegativeBits(EnumDeclBits.getNextBits(/*Width=*/8));
- bool ShouldSkipCheckingODR = EnumDeclBits.getNextBit();
ED->setScoped(EnumDeclBits.getNextBit());
ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
ED->setFixed(EnumDeclBits.getNextBit());
- if (!ShouldSkipCheckingODR) {
- ED->setHasODRHash(true);
- ED->ODRHash = Record.readInt();
- }
+ ED->setHasODRHash(true);
+ ED->ODRHash = Record.readInt();
// If this is a definition subject to the ODR, and we already have a
// definition, merge this one into it.
@@ -864,9 +861,6 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
VisitRecordDeclImpl(RD);
- // We should only reach here if we're in C/Objective-C. There is no
- // global module fragment.
- assert(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -1066,7 +1060,6 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
FD->setCachedLinkage((Linkage)FunctionDeclBits.getNextBits(/*Width=*/3));
FD->setStorageClass((StorageClass)FunctionDeclBits.getNextBits(/*Width=*/3));
- bool ShouldSkipCheckingODR = FunctionDeclBits.getNextBit();
FD->setInlineSpecified(FunctionDeclBits.getNextBit());
FD->setImplicitlyInline(FunctionDeclBits.getNextBit());
FD->setHasSkippedBody(FunctionDeclBits.getNextBit());
@@ -1096,10 +1089,8 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());
- if (!ShouldSkipCheckingODR) {
- FD->ODRHash = Record.readInt();
- FD->setHasODRHash(true);
- }
+ FD->ODRHash = Record.readInt();
+ FD->setHasODRHash(true);
if (FD->isDefaulted() || FD->isDeletedAsWritten()) {
// If 'Info' is nonzero, we need to read an DefaultedOrDeletedInfo; if,
@@ -1971,8 +1962,6 @@ void ASTDeclReader::ReadCXXDefinitionData(
BitsUnpacker CXXRecordDeclBits = Record.readInt();
- bool ShouldSkipCheckingODR = CXXRecordDeclBits.getNextBit();
-
#define FIELD(Name, Width, Merge) \
if (!CXXRecordDeclBits.canGetNextNBits(Width)) \
CXXRecordDeclBits.updateValue(Record.readInt()); \
@@ -1981,12 +1970,9 @@ void ASTDeclReader::ReadCXXDefinitionData(
#include "clang/AST/CXXRecordDeclDefinitionBits.def"
#undef FIELD
- // We only perform ODR checks for decls not in GMF.
- if (!ShouldSkipCheckingODR) {
- // Note: the caller has deserialized the IsLambda bit already.
- Data.ODRHash = Record.readInt();
- Data.HasODRHash = true;
- }
+ // Note: the caller has deserialized the IsLambda bit already.
+ Data.ODRHash = Record.readInt();
+ Data.HasODRHash = true;
if (Record.readInt()) {
Reader.DefinitionSource[D] =
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 5b5b468532f32..c78d8943d6d92 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6543,9 +6543,6 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
BitsPacker DefinitionBits;
- bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
- DefinitionBits.addBit(ShouldSkipCheckingODR);
-
#define FIELD(Name, Width, Merge) \
if (!DefinitionBits.canWriteNextNBits(Width)) { \
Record->push_back(DefinitionBits); \
@@ -6558,11 +6555,9 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
Record->push_back(DefinitionBits);
- // We only perform ODR checks for decls not in GMF.
- if (!ShouldSkipCheckingODR)
- // getODRHash will compute the ODRHash if it has not been previously
- // computed.
- Record->push_back(D->getODRHash());
+ // getODRHash will compute the ODRHash if it has not been previously
+ // computed.
+ Record->push_back(D->getODRHash());
bool ModulesDebugInfo =
Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 5dff0cec5c0ea..17c774038571e 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -527,16 +527,12 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
BitsPacker EnumDeclBits;
EnumDeclBits.addBits(D->getNumPositiveBits(), /*BitWidth=*/8);
EnumDeclBits.addBits(D->getNumNegativeBits(), /*BitWidth=*/8);
- bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
- EnumDeclBits.addBit(ShouldSkipCheckingODR);
EnumDeclBits.addBit(D->isScoped());
EnumDeclBits.addBit(D->isScopedUsingClassTag());
EnumDeclBits.addBit(D->isFixed());
Record.push_back(EnumDeclBits);
- // We only perform ODR checks for decls not in GMF.
- if (!ShouldSkipCheckingODR)
- Record.push_back(D->getODRHash());
+ Record.push_back(D->getODRHash());
if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) {
Record.AddDeclRef(MemberInfo->getInstantiatedFrom());
@@ -553,7 +549,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
!D->isTopLevelDeclInObjCContainer() &&
!CXXRecordDecl::classofKind(D->getKind()) &&
!D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() &&
- !needsAnonymousDeclarationNumber(D) && !shouldSkipCheckingODR(D) &&
+ !needsAnonymousDeclarationNumber(D) &&
D->getDeclName().getNameKind() == DeclarationName::Identifier)
AbbrevToUse = Writer.getDeclEnumAbbrev();
@@ -719,8 +715,6 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
// FIXME: stable encoding
FunctionDeclBits.addBits(llvm::to_underlying(D->getLinkageInternal()), 3);
FunctionDeclBits.addBits((uint32_t)D->getStorageClass(), /*BitWidth=*/3);
- bool ShouldSkipCheckingODR = shouldSkipCheckingODR(D);
- FunctionDeclBits.addBit(ShouldSkipCheckingODR);
FunctionDeclBits.addBit(D->isInlineSpecified());
FunctionDeclBits.addBit(D->isInlined());
FunctionDeclBits.addBit(D->hasSkippedBody());
@@ -746,9 +740,7 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
if (D->isExplicitlyDefaulted())
Record.AddSourceLocation(D->getDefaultLoc());
- // We only perform ODR checks for decls not in GMF.
- if (!ShouldSkipCheckingODR)
- Record.push_back(D->getODRHash());
+ Record.push_back(D->getODRHash());
if (D->isDefaulted() || D->isDeletedAsWritten()) {
if (auto *FDI = D->getDefalutedOrDeletedInfo()) {
@@ -1560,8 +1552,7 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) {
D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() &&
!D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() &&
D->getDeclName().getNameKind() == DeclarationName::Identifier &&
- !shouldSkipCheckingODR(D) && !D->hasExtInfo() &&
- !D->isExplicitlyDefaulted()) {
+ !D->hasExtInfo() && !D->isExplicitlyDefaulted()) {
if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization ||
More information about the cfe-commits
mailing list