Author: Ilya Biryukov Date: 2025-01-22T18:17:37+01:00 New Revision: f63e8ed16ef1fd2deb80cd88b5ca9d5b631b1c36
URL: https://github.com/llvm/llvm-project/commit/f63e8ed16ef1fd2deb80cd88b5ca9d5b631b1c36 DIFF: https://github.com/llvm/llvm-project/commit/f63e8ed16ef1fd2deb80cd88b5ca9d5b631b1c36.diff LOG: Revert "[Modules] Delay deserialization of preferred_name attribute at r… (#122726)" This reverts commit c3ba6f378ef80d750e2278560c6f95a300114412. We are seeing performance regressions of up to 40% on some compilations with this patch, we will investigate and reland after fixing performance issues. Added: Modified: clang/include/clang/AST/Attr.h clang/include/clang/Basic/Attr.td clang/include/clang/Serialization/ASTReader.h clang/include/clang/Serialization/ASTRecordReader.h clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/test/Modules/preferred_name.cppm clang/utils/TableGen/ClangAttrEmitter.cpp Removed: ################################################################################ diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h index bed532a84a1bde..3365ebe4d9012b 100644 --- a/clang/include/clang/AST/Attr.h +++ b/clang/include/clang/AST/Attr.h @@ -60,8 +60,6 @@ class Attr : public AttributeCommonInfo { unsigned IsLateParsed : 1; LLVM_PREFERRED_TYPE(bool) unsigned InheritEvenIfAlreadyPresent : 1; - LLVM_PREFERRED_TYPE(bool) - unsigned DeferDeserialization : 1; void *operator new(size_t bytes) noexcept { llvm_unreachable("Attrs cannot be allocated with regular 'new'."); @@ -82,11 +80,10 @@ class Attr : public AttributeCommonInfo { protected: Attr(ASTContext &Context, const AttributeCommonInfo &CommonInfo, - attr::Kind AK, bool IsLateParsed, bool DeferDeserialization = false) + attr::Kind AK, bool IsLateParsed) : AttributeCommonInfo(CommonInfo), AttrKind(AK), Inherited(false), IsPackExpansion(false), Implicit(false), IsLateParsed(IsLateParsed), - InheritEvenIfAlreadyPresent(false), - DeferDeserialization(DeferDeserialization) {} + InheritEvenIfAlreadyPresent(false) {} public: attr::Kind getKind() const { return static_cast<attr::Kind>(AttrKind); } @@ -108,8 +105,6 @@ class Attr : public AttributeCommonInfo { void setPackExpansion(bool PE) { IsPackExpansion = PE; } bool isPackExpansion() const { return IsPackExpansion; } - bool shouldDeferDeserialization() const { return DeferDeserialization; } - // Clone this attribute. Attr *clone(ASTContext &C) const; @@ -151,9 +146,8 @@ class InheritableAttr : public Attr { protected: InheritableAttr(ASTContext &Context, const AttributeCommonInfo &CommonInfo, attr::Kind AK, bool IsLateParsed, - bool InheritEvenIfAlreadyPresent, - bool DeferDeserialization = false) - : Attr(Context, CommonInfo, AK, IsLateParsed, DeferDeserialization) { + bool InheritEvenIfAlreadyPresent) + : Attr(Context, CommonInfo, AK, IsLateParsed) { this->InheritEvenIfAlreadyPresent = InheritEvenIfAlreadyPresent; } diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 3969dd8af5dfae..408d3adf370c85 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -713,12 +713,6 @@ class Attr { // attribute may be documented under multiple categories, more than one // Documentation entry may be listed. list<Documentation> Documentation; - // Set to true if deserialization of this attribute must be deferred until - // the parent Decl is fully deserialized (during header module file - // deserialization). E.g., this is the case for the preferred_name attribute, - // since its type deserialization depends on its target Decl type. - // (See https://github.com/llvm/llvm-project/issues/56490 for details). - bit DeferDeserialization = 0; } /// Used to define a set of mutually exclusive attributes. @@ -3260,11 +3254,6 @@ def PreferredName : InheritableAttr { let InheritEvenIfAlreadyPresent = 1; let MeaningfulToClassTemplateDefinition = 1; let TemplateDependent = 1; - // Type of this attribute depends on the target Decl type. - // Therefore, its deserialization must be deferred until - // deserialization of the target Decl is complete - // (for header modules). - let DeferDeserialization = 1; } def PreserveMost : DeclOrTypeAttr { diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 82564fe664acba..7530015c9dacf3 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1236,24 +1236,6 @@ class ASTReader /// been completed. std::deque<PendingDeclContextInfo> PendingDeclContextInfos; - /// Deserialization of some attributes must be deferred since they refer - /// to themselves in their type (e.g., preferred_name attribute refers to the - /// typedef that refers back to the template specialization of the template - /// that the attribute is attached to). - /// More attributes that store TypeSourceInfo might be potentially affected, - /// see https://github.com/llvm/llvm-project/issues/56490 for details. - struct DeferredAttribute { - // Index of the deferred attribute in the Record of the TargetedDecl. - uint64_t RecordIdx; - // Decl to attach a deferred attribute to. - Decl *TargetedDecl; - }; - - /// The collection of Decls that have been loaded but some of their attributes - /// have been deferred, paired with the index inside the record pointing - /// at the skipped attribute. - SmallVector<DeferredAttribute> PendingDeferredAttributes; - template <typename DeclTy> using DuplicateObjCDecls = std::pair<DeclTy *, DeclTy *>; @@ -1606,7 +1588,6 @@ class ASTReader void loadPendingDeclChain(Decl *D, uint64_t LocalOffset); void loadObjCCategories(GlobalDeclID ID, ObjCInterfaceDecl *D, unsigned PreviousGeneration = 0); - void loadDeferredAttribute(const DeferredAttribute &DA); RecordLocation getLocalBitOffset(uint64_t GlobalOffset); uint64_t getGlobalBitOffset(ModuleFile &M, uint64_t LocalOffset); diff --git a/clang/include/clang/Serialization/ASTRecordReader.h b/clang/include/clang/Serialization/ASTRecordReader.h index a29972fcf73a8d..2561418b78ca7f 100644 --- a/clang/include/clang/Serialization/ASTRecordReader.h +++ b/clang/include/clang/Serialization/ASTRecordReader.h @@ -83,12 +83,6 @@ class ASTRecordReader /// Returns the current value in this record, without advancing. uint64_t peekInt() { return Record[Idx]; } - /// Returns the next N values in this record, without advancing. - uint64_t peekInts(unsigned N) { return Record[Idx + N]; } - - /// Skips the current value. - void skipInt() { Idx += 1; } - /// Skips the specified number of values. void skipInts(unsigned N) { Idx += N; } @@ -341,12 +335,7 @@ class ASTRecordReader Attr *readAttr(); /// Reads attributes from the current stream position, advancing Idx. - /// For some attributes (where type depends on itself recursively), defer - /// reading the attribute until the type has been read. - void readAttributes(AttrVec &Attrs, Decl *D = nullptr); - - /// Reads one attribute from the current stream position, advancing Idx. - Attr *readOrDeferAttrFor(Decl *D); + void readAttributes(AttrVec &Attrs); /// Read an BTFTypeTagAttr object. BTFTypeTagAttr *readBTFTypeTagAttr() { diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index a72ff766685bbe..08801d22fdca86 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -10239,11 +10239,6 @@ void ASTReader::finishPendingActions() { } PendingDeducedVarTypes.clear(); - // Load the delayed preferred name attributes. - for (unsigned I = 0; I != PendingDeferredAttributes.size(); ++I) - loadDeferredAttribute(PendingDeferredAttributes[I]); - PendingDeferredAttributes.clear(); - // For each decl chain that we wanted to complete while deserializing, mark // it as "still needs to be completed". for (unsigned I = 0; I != PendingIncompleteDeclChains.size(); ++I) { diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index de834285fa76b2..72191395ec8067 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -613,7 +613,7 @@ void ASTDeclReader::VisitDecl(Decl *D) { if (HasAttrs) { AttrVec Attrs; - Record.readAttributes(Attrs, D); + Record.readAttributes(Attrs); // Avoid calling setAttrs() directly because it uses Decl::getASTContext() // internally which is unsafe during derialization. D->setAttrsImpl(Attrs, Reader.getContext()); @@ -3098,8 +3098,6 @@ class AttrReader { return Reader.readInt(); } - uint64_t peekInts(unsigned N) { return Reader.peekInts(N); } - bool readBool() { return Reader.readBool(); } SourceRange readSourceRange() { @@ -3130,29 +3128,18 @@ class AttrReader { return Reader.readVersionTuple(); } - void skipInt() { Reader.skipInts(1); } - - void skipInts(unsigned N) { Reader.skipInts(N); } - - unsigned getCurrentIdx() { return Reader.getIdx(); } - OMPTraitInfo *readOMPTraitInfo() { return Reader.readOMPTraitInfo(); } template <typename T> T *readDeclAs() { return Reader.readDeclAs<T>(); } }; } -/// Reads one attribute from the current stream position, advancing Idx. Attr *ASTRecordReader::readAttr() { AttrReader Record(*this); auto V = Record.readInt(); if (!V) return nullptr; - // Read and ignore the skip count, since attribute deserialization is not - // deferred on this pass. - Record.skipInt(); - Attr *New = nullptr; // Kind is stored as a 1-based integer because 0 is used to indicate a null // Attr pointer. @@ -3182,28 +3169,13 @@ Attr *ASTRecordReader::readAttr() { return New; } -/// Reads attributes from the current stream position, advancing Idx. -/// For some attributes (where type depends on itself recursively), defer -/// reading the attribute until the type has been read. -void ASTRecordReader::readAttributes(AttrVec &Attrs, Decl *D) { +/// Reads attributes from the current stream position. +void ASTRecordReader::readAttributes(AttrVec &Attrs) { for (unsigned I = 0, E = readInt(); I != E; ++I) - if (auto *A = readOrDeferAttrFor(D)) + if (auto *A = readAttr()) Attrs.push_back(A); } -/// Reads one attribute from the current stream position, advancing Idx. -/// For some attributes (where type depends on itself recursively), defer -/// reading the attribute until the type has been read. -Attr *ASTRecordReader::readOrDeferAttrFor(Decl *D) { - AttrReader Record(*this); - unsigned SkipCount = Record.peekInts(1); - if (!SkipCount) - return readAttr(); - Reader->PendingDeferredAttributes.push_back({Record.getCurrentIdx(), D}); - Record.skipInts(SkipCount); - return nullptr; -} - //===----------------------------------------------------------------------===// // ASTReader Implementation //===----------------------------------------------------------------------===// @@ -4512,49 +4484,6 @@ void ASTReader::loadPendingDeclChain(Decl *FirstLocal, uint64_t LocalOffset) { ASTDeclReader::attachLatestDecl(CanonDecl, MostRecent); } -void ASTReader::loadDeferredAttribute(const DeferredAttribute &DA) { - Decl *D = DA.TargetedDecl; - ModuleFile *M = getOwningModuleFile(D); - - unsigned LocalDeclIndex = D->getGlobalID().getLocalDeclIndex(); - const DeclOffset &DOffs = M->DeclOffsets[LocalDeclIndex]; - RecordLocation Loc(M, DOffs.getBitOffset(M->DeclsBlockStartOffset)); - - llvm::BitstreamCursor &Cursor = Loc.F->DeclsCursor; - SavedStreamPosition SavedPosition(Cursor); - if (llvm::Error Err = Cursor.JumpToBit(Loc.Offset)) { - Error(std::move(Err)); - } - - Expected<unsigned> MaybeCode = Cursor.ReadCode(); - if (!MaybeCode) { - llvm::report_fatal_error( - Twine("ASTReader::loadPreferredNameAttribute failed reading code: ") + - toString(MaybeCode.takeError())); - } - unsigned Code = MaybeCode.get(); - - ASTRecordReader Record(*this, *Loc.F); - Expected<unsigned> MaybeRecCode = Record.readRecord(Cursor, Code); - if (!MaybeRecCode) { - llvm::report_fatal_error( - Twine( - "ASTReader::loadPreferredNameAttribute failed reading rec code: ") + - toString(MaybeCode.takeError())); - } - unsigned RecCode = MaybeRecCode.get(); - if (RecCode < DECL_TYPEDEF || RecCode > DECL_LAST) { - llvm::report_fatal_error( - Twine("ASTReader::loadPreferredNameAttribute failed reading rec code: " - "expected valid DeclCode") + - toString(MaybeCode.takeError())); - } - - Record.skipInts(DA.RecordIdx); - Attr *A = Record.readAttr(); - getContext().getDeclAttrs(D).push_back(A); -} - namespace { /// Given an ObjC interface, goes through the modules and links to the diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 066c4b1533552a..a580f375aee354 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -37,7 +37,6 @@ #include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h" #include "clang/AST/TypeLocVisitor.h" -#include "clang/Basic/AttrKinds.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/FileEntry.h" @@ -5156,14 +5155,15 @@ void ASTWriter::WriteModuleFileExtension(Sema &SemaRef, void ASTRecordWriter::AddAttr(const Attr *A) { auto &Record = *this; - if (!A) + // FIXME: Clang can't handle the serialization/deserialization of + // preferred_name properly now. See + // https://github.com/llvm/llvm-project/issues/56490 for example. + if (!A || (isa<PreferredNameAttr>(A) && + Writer->isWritingStdCXXNamedModules())) return Record.push_back(0); Record.push_back(A->getKind() + 1); // FIXME: stable encoding, target attrs - auto SkipIdx = Record.size(); - // Add placeholder for the size of deferred attribute. - Record.push_back(0); Record.AddIdentifierRef(A->getAttrName()); Record.AddIdentifierRef(A->getScopeName()); Record.AddSourceRange(A->getRange()); @@ -5174,12 +5174,6 @@ void ASTRecordWriter::AddAttr(const Attr *A) { Record.push_back(A->isRegularKeywordAttribute()); #include "clang/Serialization/AttrPCHWrite.inc" - - if (A->shouldDeferDeserialization()) { - // Record the actual size of deferred attribute (+ 1 to count the attribute - // kind). - Record[SkipIdx] = Record.size() - SkipIdx + 1; - } } /// Emit the list of attributes to the specified record. diff --git a/clang/test/Modules/preferred_name.cppm b/clang/test/Modules/preferred_name.cppm index 86ba6ae96db998..806781a81c5ca7 100644 --- a/clang/test/Modules/preferred_name.cppm +++ b/clang/test/Modules/preferred_name.cppm @@ -53,16 +53,10 @@ import A; export using ::foo_templ; //--- Use1.cpp -// expected-no-diagnostics -import A; -#include "foo.h" +import A; // expected-warning@foo.h:8 {{attribute declaration must precede definition}} +#include "foo.h" // expected-note@foo.h:9 {{previous definition is here}} + //--- Use2.cpp // expected-no-diagnostics #include "foo.h" import A; - -//--- Use3.cpp -#include "foo.h" -import A; -foo test; -int size = test.size(); // expected-error {{no member named 'size' in 'foo'}} diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp index 41730eba32ce27..cc6a8eaebd44ec 100644 --- a/clang/utils/TableGen/ClangAttrEmitter.cpp +++ b/clang/utils/TableGen/ClangAttrEmitter.cpp @@ -3043,10 +3043,6 @@ static void emitAttributes(const RecordKeeper &Records, raw_ostream &OS, << (R.getValueAsBit("InheritEvenIfAlreadyPresent") ? "true" : "false"); } - if (R.getValueAsBit("DeferDeserialization")) { - OS << ", " - << "/*DeferDeserialization=*/true"; - } OS << ")\n"; for (auto const &ai : Args) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits