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 || _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
