Skip to content

[NFC] [Serializer] Pack information in serializer #69287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 3, 2023
Merged

[NFC] [Serializer] Pack information in serializer #69287

merged 1 commit into from
Nov 3, 2023

Conversation

ChuanqiXu9
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 commented Oct 17, 2023 •
edited
Loading

Previously, the boolean values will occupy spaces that can contain integers. It wastes the spaces especially if the boolean values are serialized consecutively. The patch tries to pack such consecutive boolean values (and enum values) so that we can save more spaces and so the times.

Before the patch, we need 4.478s (in my machine) to build the std module (https://libcxx.llvm.org/Modules.html) with 28712 bytes for size of the BMI. After the patch, the time becomes to 4.374s and the size becomes to 27388 bytes for the size of the BMI.

This is intended to be a NFC patch.

This patch doesn't optimize all such cases. We can do it later after we have consensus on this.

@ChuanqiXu9 ChuanqiXu9 added the clang:modules C++20 modules and Clang Header Modules label Oct 17, 2023
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 17, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2023 •
edited
Loading

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Previously, the boolean values will occupy spaces that can contain integers. It wastes the spaces especially if the boolean values are serialized consecutively. The patch tries to pack such consecutive boolean values (and enum values) so that we can save more spaces and so the times.

Before the patch, we need 4.478s (in my machine) to build the std module (https://libcxx.llvm.org/Modules.html) with 28712 bytes. After the patch, the time becomes to 4.374s and the size becomes to 27568 bytes.

This is intended to be a NFC patch.

This patch doesn't optimize all such cases. We can do it later after we have consensus on this.

Patch is 45.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/69287.diff

5 Files Affected:

  • (modified) clang/include/clang/AST/Decl.h (+1-1)
  • (modified) clang/include/clang/AST/DeclBase.h (+1-1)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+103-88)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+155-240)
  • (modified) clang/test/Modules/decl-params-determinisim.m (+8-8)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 7f076cc77ea82cb..cd9a830dbaaf426 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -4073,7 +4073,7 @@ class RecordDecl : public TagDecl {
   /// returned from function calls. This takes into account the target-specific
   /// and version-specific rules along with the rules determined by the
   /// language.
-  enum ArgPassingKind : unsigned {
+  enum ArgPassingKind : unsigned char {
     /// The argument of this type can be passed directly in registers.
     APK_CanPassInRegs,
 
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index d383e46e22e16f4..7a30529f1f73d74 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -211,7 +211,7 @@ class alignas(8) Decl {
   /// The kind of ownership a declaration has, for visibility purposes.
   /// This enumeration is designed such that higher values represent higher
   /// levels of name hiding.
-  enum class ModuleOwnershipKind : unsigned {
+  enum class ModuleOwnershipKind : unsigned char {
     /// This declaration is not owned by a module.
     Unowned,
 
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 3a3477c39efae25..62b2665c2211029 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -617,24 +617,27 @@ void ASTDeclReader::VisitDecl(Decl *D) {
                            Reader.getContext());
   }
   D->setLocation(ThisDeclLoc);
-  D->InvalidDecl = Record.readInt();
-  if (Record.readInt()) { // hasAttrs
+
+  uint64_t DeclBits = Record.readInt();
+  D->InvalidDecl = DeclBits & 0x1;
+  D->setImplicit(DeclBits & (1 << 2));
+  D->Used = (DeclBits >> 3) & 0x1;
+  IsDeclMarkedUsed |= D->Used;
+  D->setReferenced(DeclBits & (1 << 4));
+  D->setTopLevelDeclInObjCContainer(DeclBits & (1 << 5));
+  D->setAccess((AccessSpecifier)((DeclBits >> 6) & 0x3));
+  D->FromASTFile = true;
+  auto ModuleOwnership = (Decl::ModuleOwnershipKind)((DeclBits >> 8) & 0x7);
+  bool ModulePrivate =
+      (ModuleOwnership == Decl::ModuleOwnershipKind::ModulePrivate);
+
+  if (DeclBits & (0x1 << 1)) { // hasAttrs
     AttrVec Attrs;
     Record.readAttributes(Attrs);
     // Avoid calling setAttrs() directly because it uses Decl::getASTContext()
     // internally which is unsafe during derialization.
     D->setAttrsImpl(Attrs, Reader.getContext());
   }
-  D->setImplicit(Record.readInt());
-  D->Used = Record.readInt();
-  IsDeclMarkedUsed |= D->Used;
-  D->setReferenced(Record.readInt());
-  D->setTopLevelDeclInObjCContainer(Record.readInt());
-  D->setAccess((AccessSpecifier)Record.readInt());
-  D->FromASTFile = true;
-  auto ModuleOwnership = (Decl::ModuleOwnershipKind)Record.readInt();
-  bool ModulePrivate =
-      (ModuleOwnership == Decl::ModuleOwnershipKind::ModulePrivate);
 
   // Determine whether this declaration is part of a (sub)module. If so, it
   // may not yet be visible.
@@ -750,12 +753,14 @@ ASTDeclReader::RedeclarableResult ASTDeclReader::VisitTagDecl(TagDecl *TD) {
   VisitTypeDecl(TD);
 
   TD->IdentifierNamespace = Record.readInt();
-  TD->setTagKind((TagDecl::TagKind)Record.readInt());
+
+  uint32_t TagDeclBits = Record.readInt();
+  TD->setTagKind((TagDecl::TagKind)(TagDeclBits & 0x7));
   if (!isa<CXXRecordDecl>(TD))
-    TD->setCompleteDefinition(Record.readInt());
-  TD->setEmbeddedInDeclarator(Record.readInt());
-  TD->setFreeStanding(Record.readInt());
-  TD->setCompleteDefinitionRequired(Record.readInt());
+    TD->setCompleteDefinition(TagDeclBits & (0x1 << 3));
+  TD->setEmbeddedInDeclarator(TagDeclBits & (0x1 << 4));
+  TD->setFreeStanding(TagDeclBits & (0x1 << 5));
+  TD->setCompleteDefinitionRequired(TagDeclBits & (0x1 << 6));
   TD->setBraceRange(readSourceRange());
 
   switch (Record.readInt()) {
@@ -787,11 +792,13 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
   else
     ED->setIntegerType(Record.readType());
   ED->setPromotionType(Record.readType());
-  ED->setNumPositiveBits(Record.readInt());
-  ED->setNumNegativeBits(Record.readInt());
-  ED->setScoped(Record.readInt());
-  ED->setScopedUsingClassTag(Record.readInt());
-  ED->setFixed(Record.readInt());
+
+  uint32_t EnumDeclBits = Record.readInt();
+  ED->setNumPositiveBits(EnumDeclBits & 0xff);
+  ED->setNumNegativeBits((EnumDeclBits >> 8) & 0xff);
+  ED->setScoped(EnumDeclBits & (1 << 16));
+  ED->setScopedUsingClassTag(EnumDeclBits & (1 << 17));
+  ED->setFixed(EnumDeclBits & (1 << 18));
 
   ED->setHasODRHash(true);
   ED->ODRHash = Record.readInt();
@@ -834,18 +841,20 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
 ASTDeclReader::RedeclarableResult
 ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
   RedeclarableResult Redecl = VisitTagDecl(RD);
-  RD->setHasFlexibleArrayMember(Record.readInt());
-  RD->setAnonymousStructOrUnion(Record.readInt());
-  RD->setHasObjectMember(Record.readInt());
-  RD->setHasVolatileMember(Record.readInt());
-  RD->setNonTrivialToPrimitiveDefaultInitialize(Record.readInt());
-  RD->setNonTrivialToPrimitiveCopy(Record.readInt());
-  RD->setNonTrivialToPrimitiveDestroy(Record.readInt());
-  RD->setHasNonTrivialToPrimitiveDefaultInitializeCUnion(Record.readInt());
-  RD->setHasNonTrivialToPrimitiveDestructCUnion(Record.readInt());
-  RD->setHasNonTrivialToPrimitiveCopyCUnion(Record.readInt());
-  RD->setParamDestroyedInCallee(Record.readInt());
-  RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)Record.readInt());
+
+  uint32_t RecordDeclBits = Record.readInt();
+  RD->setHasFlexibleArrayMember(RecordDeclBits & 0x1);
+  RD->setAnonymousStructOrUnion(RecordDeclBits & (1 << 1));
+  RD->setHasObjectMember(RecordDeclBits & (1 << 2));
+  RD->setHasVolatileMember(RecordDeclBits & (1 << 3));
+  RD->setNonTrivialToPrimitiveDefaultInitialize(RecordDeclBits & (1 << 4));
+  RD->setNonTrivialToPrimitiveCopy(RecordDeclBits & (1 << 5));
+  RD->setNonTrivialToPrimitiveDestroy(RecordDeclBits & (1 << 6));
+  RD->setHasNonTrivialToPrimitiveDefaultInitializeCUnion(RecordDeclBits & (1 << 7));
+  RD->setHasNonTrivialToPrimitiveDestructCUnion(RecordDeclBits & (1 << 8));
+  RD->setHasNonTrivialToPrimitiveCopyCUnion(RecordDeclBits & (1 << 9));
+  RD->setParamDestroyedInCallee(RecordDeclBits & (1 << 10));
+  RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)((RecordDeclBits >> 11) & 0x3));
   return Redecl;
 }
 
@@ -1046,32 +1055,33 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
 
   // FunctionDecl's body is handled last at ASTDeclReader::Visit,
   // after everything else is read.
+  uint64_t FunctionDeclBits = Record.readInt();
 
-  FD->setStorageClass(static_cast<StorageClass>(Record.readInt()));
-  FD->setInlineSpecified(Record.readInt());
-  FD->setImplicitlyInline(Record.readInt());
-  FD->setVirtualAsWritten(Record.readInt());
+  FD->setStorageClass(static_cast<StorageClass>(FunctionDeclBits & 0x7));
+  FD->setInlineSpecified(FunctionDeclBits & (1 << 3));
+  FD->setImplicitlyInline(FunctionDeclBits & (1 << 4));
+  FD->setVirtualAsWritten(FunctionDeclBits & (1 << 5));
   // We defer calling `FunctionDecl::setPure()` here as for methods of
   // `CXXTemplateSpecializationDecl`s, we may not have connected up the
   // definition (which is required for `setPure`).
-  const bool Pure = Record.readInt();
-  FD->setHasInheritedPrototype(Record.readInt());
-  FD->setHasWrittenPrototype(Record.readInt());
-  FD->setDeletedAsWritten(Record.readInt());
-  FD->setTrivial(Record.readInt());
-  FD->setTrivialForCall(Record.readInt());
-  FD->setDefaulted(Record.readInt());
-  FD->setExplicitlyDefaulted(Record.readInt());
-  FD->setIneligibleOrNotSelected(Record.readInt());
-  FD->setHasImplicitReturnZero(Record.readInt());
-  FD->setConstexprKind(static_cast<ConstexprSpecKind>(Record.readInt()));
-  FD->setUsesSEHTry(Record.readInt());
-  FD->setHasSkippedBody(Record.readInt());
-  FD->setIsMultiVersion(Record.readInt());
-  FD->setLateTemplateParsed(Record.readInt());
-  FD->setFriendConstraintRefersToEnclosingTemplate(Record.readInt());
-
-  FD->setCachedLinkage(static_cast<Linkage>(Record.readInt()));
+  const bool Pure = FunctionDeclBits & (1 << 6);
+  FD->setHasInheritedPrototype(FunctionDeclBits & (1 << 7));
+  FD->setHasWrittenPrototype(FunctionDeclBits & (1 << 8));
+  FD->setDeletedAsWritten(FunctionDeclBits & (1 << 9));
+  FD->setTrivial(FunctionDeclBits & (1 << 10));
+  FD->setTrivialForCall(FunctionDeclBits & (1 << 11));
+  FD->setDefaulted(FunctionDeclBits & (1 << 12));
+  FD->setExplicitlyDefaulted(FunctionDeclBits & (1 << 13));
+  FD->setIneligibleOrNotSelected(FunctionDeclBits & (1 << 14));
+  FD->setHasImplicitReturnZero(FunctionDeclBits & (1 << 15));
+  FD->setConstexprKind(static_cast<ConstexprSpecKind>((FunctionDeclBits >> 16) & 0x3));
+  FD->setUsesSEHTry(FunctionDeclBits & (1 << 18));
+  FD->setHasSkippedBody(FunctionDeclBits & (1 << 19));
+  FD->setIsMultiVersion(FunctionDeclBits & (1 << 20));
+  FD->setLateTemplateParsed(FunctionDeclBits & (1 << 21));
+  FD->setFriendConstraintRefersToEnclosingTemplate(FunctionDeclBits & (1 << 22));
+  FD->setCachedLinkage(static_cast<Linkage>((FunctionDeclBits >> 23) & 0x7));
+
   FD->EndRangeLoc = readSourceLocation();
   FD->setDefaultLoc(readSourceLocation());
 
@@ -1574,26 +1584,27 @@ ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
   RedeclarableResult Redecl = VisitRedeclarable(VD);
   VisitDeclaratorDecl(VD);
 
-  VD->VarDeclBits.SClass = (StorageClass)Record.readInt();
-  VD->VarDeclBits.TSCSpec = Record.readInt();
-  VD->VarDeclBits.InitStyle = Record.readInt();
-  VD->VarDeclBits.ARCPseudoStrong = Record.readInt();
+  uint32_t RecordDeclBits = Record.readInt();
+  VD->VarDeclBits.SClass = (StorageClass)(RecordDeclBits & 0x7);
+  VD->VarDeclBits.TSCSpec = (RecordDeclBits >> 3) & 0x3;
+  VD->VarDeclBits.InitStyle = (RecordDeclBits >> 5) & 0x3;
+  VD->VarDeclBits.ARCPseudoStrong = (bool)(RecordDeclBits & (1 << 7));
   bool HasDeducedType = false;
   if (!isa<ParmVarDecl>(VD)) {
     VD->NonParmVarDeclBits.IsThisDeclarationADemotedDefinition =
-        Record.readInt();
-    VD->NonParmVarDeclBits.ExceptionVar = Record.readInt();
-    VD->NonParmVarDeclBits.NRVOVariable = Record.readInt();
-    VD->NonParmVarDeclBits.CXXForRangeDecl = Record.readInt();
-    VD->NonParmVarDeclBits.ObjCForDecl = Record.readInt();
-    VD->NonParmVarDeclBits.IsInline = Record.readInt();
-    VD->NonParmVarDeclBits.IsInlineSpecified = Record.readInt();
-    VD->NonParmVarDeclBits.IsConstexpr = Record.readInt();
-    VD->NonParmVarDeclBits.IsInitCapture = Record.readInt();
-    VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope = Record.readInt();
-    VD->NonParmVarDeclBits.ImplicitParamKind = Record.readInt();
-    VD->NonParmVarDeclBits.EscapingByref = Record.readInt();
-    HasDeducedType = Record.readInt();
+         (bool)(RecordDeclBits & (1 << 8));
+    VD->NonParmVarDeclBits.ExceptionVar = (bool)(RecordDeclBits & (1 << 9));
+    VD->NonParmVarDeclBits.NRVOVariable = (bool)(RecordDeclBits & (1 << 10));
+    VD->NonParmVarDeclBits.CXXForRangeDecl = (bool)(RecordDeclBits & (1 << 11));
+    VD->NonParmVarDeclBits.ObjCForDecl = (bool)(RecordDeclBits & (1 << 12));
+    VD->NonParmVarDeclBits.IsInline = (bool)(RecordDeclBits & (1 << 13));
+    VD->NonParmVarDeclBits.IsInlineSpecified = (bool)(RecordDeclBits & (1 << 14));
+    VD->NonParmVarDeclBits.IsConstexpr = (bool)(RecordDeclBits & (1 << 15));
+    VD->NonParmVarDeclBits.IsInitCapture = (bool)(RecordDeclBits & (1 << 16));
+    VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope = (bool)(RecordDeclBits & (1 << 17));
+    VD->NonParmVarDeclBits.ImplicitParamKind = (RecordDeclBits >> 18) & 0x7;
+    VD->NonParmVarDeclBits.EscapingByref = (bool)(RecordDeclBits & (1 << 21));
+    HasDeducedType = (bool)(RecordDeclBits & (1 << 22));
   }
 
   // If this variable has a deduced type, defer reading that type until we are
@@ -1605,7 +1616,7 @@ ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
     VD->setType(Reader.GetType(DeferredTypeID));
   DeferredTypeID = 0;
 
-  auto VarLinkage = Linkage(Record.readInt());
+  auto VarLinkage = Linkage((RecordDeclBits >> 23) & 0x7);
   VD->setCachedLinkage(VarLinkage);
 
   // Reconstruct the one piece of the IdentifierNamespace that we need.
@@ -1613,18 +1624,18 @@ ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
       VD->getLexicalDeclContext()->isFunctionOrMethod())
     VD->setLocalExternDecl();
 
+  if (RecordDeclBits & (1 << 26)) {
+    Reader.DefinitionSource[VD] =
+        Loc.F->Kind == ModuleKind::MK_MainFile ||
+        Reader.getContext().getLangOpts().BuildingPCHWithObjectFile;
+  }
+
   if (VD->hasAttr<BlocksAttr>()) {
     Expr *CopyExpr = Record.readExpr();
     if (CopyExpr)
       Reader.getContext().setBlockVarCopyInit(VD, CopyExpr, Record.readInt());
   }
 
-  if (Record.readInt()) {
-    Reader.DefinitionSource[VD] =
-        Loc.F->Kind == ModuleKind::MK_MainFile ||
-        Reader.getContext().getLangOpts().BuildingPCHWithObjectFile;
-  }
-
   enum VarKind {
     VarNotTemplate = 0, VarTemplate, StaticDataMemberSpecialization
   };
@@ -1678,9 +1689,11 @@ void ASTDeclReader::VisitImplicitParamDecl(ImplicitParamDecl *PD) {
 
 void ASTDeclReader::VisitParmVarDecl(ParmVarDecl *PD) {
   VisitVarDecl(PD);
-  unsigned isObjCMethodParam = Record.readInt();
-  unsigned scopeDepth = Record.readInt();
-  unsigned scopeIndex = Record.readInt();
+
+  uint32_t ParmVarDeclBits = Record.readInt();
+  unsigned isObjCMethodParam = ParmVarDeclBits & 0x1;
+  unsigned scopeDepth = (ParmVarDeclBits >> 1) & 0x7f;
+  unsigned scopeIndex = (ParmVarDeclBits >> 8) & 0xff;
   unsigned declQualifier = Record.readInt();
   if (isObjCMethodParam) {
     assert(scopeDepth == 0);
@@ -1689,9 +1702,9 @@ void ASTDeclReader::VisitParmVarDecl(ParmVarDecl *PD) {
   } else {
     PD->setScopeInfo(scopeDepth, scopeIndex);
   }
-  PD->ParmVarDeclBits.IsKNRPromoted = Record.readInt();
-  PD->ParmVarDeclBits.HasInheritedDefaultArg = Record.readInt();
-  if (Record.readInt()) // hasUninstantiatedDefaultArg.
+  PD->ParmVarDeclBits.IsKNRPromoted = (bool)(ParmVarDeclBits & (1 << 16));
+  PD->ParmVarDeclBits.HasInheritedDefaultArg = (bool)(ParmVarDeclBits & (1 << 17));
+  if ((bool)(ParmVarDeclBits & (1 << 18))) // hasUninstantiatedDefaultArg.
     PD->setUninstantiatedDefaultArg(Record.readExpr());
   PD->ExplicitObjectParameterIntroducerLoc = Record.readSourceLocation();
 
@@ -1790,8 +1803,10 @@ void ASTDeclReader::VisitLabelDecl(LabelDecl *D) {
 void ASTDeclReader::VisitNamespaceDecl(NamespaceDecl *D) {
   RedeclarableResult Redecl = VisitRedeclarable(D);
   VisitNamedDecl(D);
-  D->setInline(Record.readInt());
-  D->setNested(Record.readInt());
+
+  uint32_t NamespaceDeclBits = Record.readInt();
+  D->setInline(NamespaceDeclBits & 0x1);
+  D->setNested(NamespaceDeclBits & 0x2);
   D->LocStart = readSourceLocation();
   D->RBraceLoc = readSourceLocation();
 
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 8a2ea7c7624ceb8..96cdfec14257c98 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -325,16 +325,22 @@ void ASTDeclWriter::VisitDecl(Decl *D) {
     Record.AddDeclRef(cast_or_null<Decl>(D->getLexicalDeclContext()));
   else
     Record.push_back(0);
-  Record.push_back(D->isInvalidDecl());
-  Record.push_back(D->hasAttrs());
+
+  uint64_t DeclBits = 0;
+  DeclBits = D->isInvalidDecl();
+  DeclBits |= D->hasAttrs() << 1;
+  DeclBits |= D->isImplicit() << 2;
+  DeclBits |= D->isUsed(false) << 3;
+  DeclBits |= D->isReferenced() << 4;
+  DeclBits |= D->isTopLevelDeclInObjCContainer() << 5;
+  assert(D->getAccess() < 4);
+  DeclBits |= D->getAccess() << 6;
+  DeclBits |= (uint64_t)D->getModuleOwnershipKind() << 8;
+  Record.push_back(DeclBits);
+
   if (D->hasAttrs())
     Record.AddAttributes(D->getAttrs());
-  Record.push_back(D->isImplicit());
-  Record.push_back(D->isUsed(false));
-  Record.push_back(D->isReferenced());
-  Record.push_back(D->isTopLevelDeclInObjCContainer());
-  Record.push_back(D->getAccess());
-  Record.push_back((uint64_t)D->getModuleOwnershipKind());
+
   Record.push_back(Writer.getSubmoduleID(D->getOwningModule()));
 
   // If this declaration injected a name into a context different from its
@@ -438,12 +444,17 @@ void ASTDeclWriter::VisitTagDecl(TagDecl *D) {
   VisitRedeclarable(D);
   VisitTypeDecl(D);
   Record.push_back(D->getIdentifierNamespace());
-  Record.push_back((unsigned)D->getTagKind()); // FIXME: stable encoding
+
+  uint32_t TagDeclBits = 0;
+  assert(D->getTagKind() < 8);
+  TagDeclBits = D->getTagKind();
   if (!isa<CXXRecordDecl>(D))
-    Record.push_back(D->isCompleteDefinition());
-  Record.push_back(D->isEmbeddedInDeclarator());
-  Record.push_back(D->isFreeStanding());
-  Record.push_back(D->isCompleteDefinitionRequired());
+    TagDeclBits |= D->isCompleteDefinition() << 3;
+  TagDeclBits |= D->isEmbeddedInDeclarator() << 4;
+  TagDeclBits |= D->isFreeStanding() << 5;
+  TagDeclBits |= D->isCompleteDefinitionRequired() << 6;
+  Record.push_back(TagDeclBits);
+
   Record.AddSourceRange(D->getBraceRange());
 
   if (D->hasExtInfo()) {
@@ -468,11 +479,17 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
   if (!D->getIntegerTypeSourceInfo())
     Record.AddTypeRef(D->getIntegerType());
   Record.AddTypeRef(D->getPromotionType());
-  Record.push_back(D->getNumPositiveBits());
-  Record.push_back(D->getNumNegativeBits());
-  Record.push_back(D->isScoped());
-  Record.push_back(D->isScopedUsingClassTag());
-  Record.push_back(D->isFixed());
+
+  uint32_t EnumDeclBits = 0;
+  assert(D->getNumPositiveBits() < (2 << 8));
+  EnumDeclBits = D->getNumPositiveBits();
+  assert(D->getNumNegativeBits() < (2 << 8));
+  EnumDeclBits |= D->getNumNegativeBits() << 8;
+  EnumDeclBits |= D->isScoped() << 16;
+  EnumDeclBits |= D->isScopedUsingClassTag() << 17;
+  EnumDeclBits |= D->isFixed() << 18;
+  Record.push_back(EnumDeclBits);
+
   Record.push_back(D->getODRHash());
 
   if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) {
@@ -511,18 +528,23 @@ void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) {
                 "RecordDeclBits");
 
   VisitTagDecl(D);
-  Record.push_back(D->hasFlexibleArrayMember());
-  Record.push_back(D->isAnonymousStructOrUnion());
-  Record.push_back(D->hasObjectMember());
-  Record.push_back(D->hasVolatileMember());
-  Record.push_back(D->isNonTrivialToPrimitiveDefaultInitialize());
-  Record.push_back(D->isNonTrivialToPrimitiveCopy());
-  Record.push_back(D->isNonTrivialToPrimitiveDestroy());
-  Record.push_back(D->hasNonTrivialToPrimitiveDefaultInitializeCUnion());
-  Record.push_back(D->hasNonTrivialToPrimitiveDestructCUnion());
-  Record.push_back(D->hasNonTrivialToPrimitiveCopyCUnion());
-  Record.push_back(D->isParamDestroyedInCallee());
-  Record.push_back(D->getArgPassingRestrictions());
+
+  uint32_t RecordDeclBits = 0;
+  RecordDeclBits = D->hasFlexibleArrayMember();
+  RecordDeclBits |= D->isAnonymousStructOrUnion() << 1;
+  RecordDeclBits |= D->hasObjectMember() << 2;
+  RecordDeclBits |= D->hasVolatileMember() << 3;
+  RecordDeclBits |= D->isNonTrivialToPrimitiveDefaultInitialize() << 4;
+  RecordDeclBits |= D->isNonTrivialToPrimitiveCopy() << 5;
+  RecordDeclBits |= D->isNonTrivialToPrimitiveDestroy() << 6;
+  RecordDeclBits |= D->hasNonTrivialToPrimitiveDefaultInitializeCUnion() << 7;
+  RecordDeclBits |= D->hasNonTrivialToPrimitiveDestructCUnion() << 8;
+  RecordDeclBits |= D->hasNonTrivialToPrimitiveCopyCUnion() << 9;
+  RecordDeclBits |= D->isParamDestroyedInCallee() << 10;
+  assert(D->getArgPassingRestrictions() < 4);
+  RecordDeclBits |= D->getArgPassingRestrictions() << 11;
+  Record.push_back(RecordDeclBits);
+
   // Only compute this for C/Objective-C, in C++ this is computed as part
   // of CXXRecordDecl.
   if (!isa<CXXRecordDecl>(D))
@@ -662,28 +684,34 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
 
   // FunctionDecl's body is handled last at ASTWriterDecl::Visit,
   // after everything else is written.
-  Record.push_back(
-      static_cast<int>(D->getStorageClass())); // FIXME: stable encoding
-  Record.push_back(D->isInlineSpecified());
-  Record.push_back(D->isInlined());
-  Record.pus...
[truncated]

@ChuanqiXu9 ChuanqiXu9 force-pushed the PackSerializationBits branch from d6a724d to 326bc3a Compare October 17, 2023 05:23
@github-actions
Copy link

github-actions bot commented Oct 17, 2023 •
edited
Loading

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command: 
git-clang-format --diff 03110ddeb2c21d1b500e7f6f6e70134e269738bc c841e9cbd9510c401def4d10df6da408ae496180 -- clang/include/clang/AST/DeclBase.h clang/include/clang/Serialization/ASTReader.h clang/include/clang/Serialization/ASTWriter.h clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp clang/lib/Serialization/ASTWriterDecl.cpp
View the diff from clang-format here. 
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a63911cb4adf..7934b349ec56 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1954,7 +1954,7 @@ void ASTDeclReader::ReadCXXDefinitionData(
   BitsUnpacker CXXRecordDeclBits = Record.readInt();
 
 #define FIELD(Name, Width, Merge)                                              \
-  if (!CXXRecordDeclBits.canGetNextNBits(Width))                         \
+  if (!CXXRecordDeclBits.canGetNextNBits(Width))                               \
     CXXRecordDeclBits.updateValue(Record.readInt());                           \
   Data.Name = CXXRecordDeclBits.getNextBits(Width);
 
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index b4438e4cf6a0..a1de204f9adf 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -536,7 +536,8 @@ void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) {
   RecordDeclBits.addBit(D->hasNonTrivialToPrimitiveDestructCUnion());
   RecordDeclBits.addBit(D->hasNonTrivialToPrimitiveCopyCUnion());
   RecordDeclBits.addBit(D->isParamDestroyedInCallee());
-  RecordDeclBits.addBits(llvm::to_underlying(D->getArgPassingRestrictions()), 2);
+  RecordDeclBits.addBits(llvm::to_underlying(D->getArgPassingRestrictions()),
+                         2);
   Record.push_back(RecordDeclBits);
 
   // Only compute this for C/Objective-C, in C++ this is computed as part
@@ -1090,7 +1091,8 @@ void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
     VarDeclBits.addBit(HasDeducedType);
   }
 
-  VarDeclBits.addBits(llvm::to_underlying(D->getLinkageInternal()), /*BitWidth=*/3);
+  VarDeclBits.addBits(llvm::to_underlying(D->getLinkageInternal()),
+                      /*BitWidth=*/3);
 
   bool ModulesCodegen = false;
   if (Writer.WritingModule && D->getStorageDuration() == SD_Static &&
@@ -2216,13 +2218,13 @@ void ASTWriter::WriteDeclAbbrevs() {
       BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,
                       12)); // Packed Var Decl bits: SClass, TSCSpec, InitStyle,
                             // isARCPseudoStrong, Linkage, ModulesCodegen
-  Abv->Add(BitCodeAbbrevOp(0));                          // VarKind (local enum)
+  Abv->Add(BitCodeAbbrevOp(0)); // VarKind (local enum)
   // ParmVarDecl
   Abv->Add(BitCodeAbbrevOp(
       BitCodeAbbrevOp::Fixed,
       19)); // Packed Parm Var Decl bits: IsObjCMethodParameter, ScopeDepth,
             // ScopeIndex, KNRPromoted, HasInheritedDefaultArg
-  Abv->Add(BitCodeAbbrevOp(0));                       // ObjCDeclQualifier
+  Abv->Add(BitCodeAbbrevOp(0));                   // ObjCDeclQualifier
   Abv->Add(BitCodeAbbrevOp(0));                   // HasUninstantiatedDefaultArg
   // Type Source Info
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));

@ChuanqiXu9 ChuanqiXu9 force-pushed the PackSerializationBits branch from 326bc3a to 5a1f32f Compare October 17, 2023 06:09
@ChuanqiXu9
Copy link
Member Author

@zygoloid @vgvassilev ping~

Personally I feel like the change itself should be safe. (The test for modules is really sensitive about the changes in Serializer)

vgvassilev
vgvassilev reviewed Oct 24, 2023
View reviewed changes
clang/lib/Serialization/ASTReaderDecl.cpp Outdated
Comment on lines 621 to 630
uint64_t DeclBits = Record.readInt();
D->InvalidDecl = DeclBits & 0x1;
D->setImplicit(DeclBits & (1 << 2));
D->Used = (DeclBits >> 3) & 0x1;
IsDeclMarkedUsed |= D->Used;
D->setReferenced(DeclBits & (1 << 4));
D->setTopLevelDeclInObjCContainer(DeclBits & (1 << 5));
D->setAccess((AccessSpecifier)((DeclBits >> 6) & 0x3));
D->FromASTFile = true;
auto ModuleOwnership = (Decl::ModuleOwnershipKind)((DeclBits >> 8) & 0x7);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to improve the API avoiding these magic constants? I suspect this would be difficult to read and maintain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I had in mind is to introduce a BitPacker, but it can't avoid magic constants completely. e.g, we have to know that the width of AccessSpecifier should be 2 and the width of ModuleOwnershipKind should be 3.

Then we can refactor the above section to:

BitPacker Bits = Record.readInt();
D->InvalidDecl = Bits.getNextBit();
D->setImplicit(Bits.getNextBit());
D->Used = Bits.getNextBit();
IsDeclMarkedUsed |= D->Used;
D->setReferenced(Bits.getNextBit());
D->setTopLevelDeclInObjCContainer(Bits.getNextBit());
D->setAccess((AccessSpecifier)Bits.getNextBit</*width = */2>()); // Still we can't avoid the magic constants completely.
D->FromASTFile = true;
auto ModuleOwnership = (Decl::ModuleOwnershipKind)Bits.getNextBit</*width = */3>());

Would you be happy about the above form?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks more readable to me. In the case of ModuleOwnershipKind can we use the bit size either with sizeof or via NextInContextAndBits? If we use sizeof this could look like:
auto ModuleOwnership = Bits.getNextBit<Decl::ModuleOwnershipKind>()); but we will likely read/write 8 bits instead of 3 in this case...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... and I feel the magic numbers for some types (e.g., AccessSpecifier and ModuleOwnershipKind) might not be so scaring. Since the devs can see AccessSpecifier and ModuleOwnershipKind here then they can get the size quickly. Also even if now we have to look at both ASTWriterDecl.cpp and ASTReaderDecl.cpp to change anything. I mean, it is already tightly bounded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ChuanqiXu9 ChuanqiXu9 force-pushed the PackSerializationBits branch 2 times, most recently from e99fc1a to ae4ee48 Compare October 26, 2023 09:45
vgvassilev
vgvassilev reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good modulo the comments.

@zygoloid ping.

vgvassilev
vgvassilev reviewed Oct 31, 2023
View reviewed changes
clang/lib/Serialization/ASTWriterDecl.cpp Outdated
Abv->Add(BitCodeAbbrevOp(0)); // TopLevelDeclInObjCContainer
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // AccessSpecifier
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // ModuleOwnershipKind
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 11)); // Decl Bits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These sections became a little harder to read. Perhaps we could keep the comments and this way the reader could probably do the math why this is 11 bits for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ChuanqiXu9
ChuanqiXu9 commented Oct 31, 2023
View reviewed changes
clang/lib/Serialization/ASTWriterDecl.cpp
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // ModuleOwnershipKind
Abv->Add(BitCodeAbbrevOp(
BitCodeAbbrevOp::Fixed,
11)); // Packed DeclBits: isInvalidDecl, HasAttrs, isImplicit, isUsed,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd at the first sight. But it is formatted by clang-format. And it looks not bad after looking into it.

@ChuanqiXu9
Copy link
Member Author

I'd like to land this in the next week if no objection come in. Since this may be safe.

vgvassilev
vgvassilev approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@ChuanqiXu9
[NFC] [Serializer] Pack information in serializer
c841e9c 
Previously, the boolean values will occupy spaces that can contain
integers. It wastes the spaces especially if the boolean values are
serialized consecutively. The patch tries to pack such consecutive
boolean values (and enum values) so that we can save more spaces and so
the times.

Before the patch, we need 4.478s (in my machine) to build the std module
(https://libcxx.llvm.org/Modules.html) with 28712 bytes for size of the
BMI. After the patch, the time becomes to 4.374s and the size becomes to
27388 bytes for the size of the BMI.

This is intended to be a NFC patch.

This patch doesn't optimize all such cases. We can do it later after we
have consensus on this.
@ChuanqiXu9 ChuanqiXu9 force-pushed the PackSerializationBits branch from e43e89b to c841e9c Compare November 3, 2023 13:59
@ChuanqiXu9
Copy link
Member Author

Thanks for reviewing : )

@ChuanqiXu9 ChuanqiXu9 merged commit 48be81e into llvm:main Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vgvassilev vgvassilev vgvassilev approved these changes

@zygoloid zygoloid Awaiting requested review from zygoloid

@Bigcheese Bigcheese Awaiting requested review from Bigcheese

@dwblaikie dwblaikie Awaiting requested review from dwblaikie

Assignees
No one assigned
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ChuanqiXu9 @llvmbot @vgvassilev