Revert "[Modules] Delay deserialization of preferred_name attribute at r…" #123962
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: Viktoriia Bakalova (VitaNuo) ChangesReverts llvm/llvm-project#122726 We're seeing a 8-10% performance regression due to this patch. Full diff: https://github.com/llvm/llvm-project/pull/123962.diff 9 Files Affected:
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; // [email protected]:8 {{attribute declaration must precede definition}}
+#include "foo.h" // [email protected]: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) {
|
|
|
You can test this locally with the following command:git-clang-format --diff 5a9b74d20d5f3b7f92c01d68d28778108dfb1308 1d94f0a4598ec08cd75fb7c1c7353ce318da2472 --extensions h,cppm,cpp -- clang/include/clang/AST/Attr.h 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.cppView the diff from clang-format here.diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index a580f375ae..69e326ee9c 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -5158,8 +5158,8 @@ void ASTRecordWriter::AddAttr(const Attr *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()))
+ if (!A ||
+ (isa<PreferredNameAttr>(A) && Writer->isWritingStdCXXNamedModules()))
return Record.push_back(0);
Record.push_back(A->getKind() + 1); // FIXME: stable encoding, target attrs
|
|
Some benchmarks show an even bigger regression of up to 40% (although we're not sure if they're clear from noise). |
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.
Reverts #122726
We're seeing a 8-10% performance regression due to this patch.