Make CompoundStmtBitfields::NumStmts not a bit-field

[sepavloff]

Summary

Number of statements in CompoundStmt is kept in a bit-field of the common
part of Stmt. The field has 24 bits for the number. To allocate a new
bit field (as attempted in https://reviews.llvm.org/D123952), this
number must be reduced, maximal number of statements in a compound
statement becomes smaller. It can result in compilation errors of some
programs.

With this change the number of statements is kept in a field of type
'unsigned int' rather than in bit-field. To make room in CompoundStmtBitfields
LBraceLoc is moved to fields of CompoundStmt.

[rjmccall]

On the one hand, I'm not sure 8M statements in a block — or 1M, for that matter — is an unreasonable implementation limit. On the other hand, this patch looks like it only changes overall memory use on 32-bit hosts, because CompoundStmt has a 32-bit field prior to its pointer-aligned trailing storage. Can you check how memory use actually changes? Ideally you'd be able to test it on a 32-bit host, but if you don't have one, just knowing the raw count of allocated CompoundStmt objects for a few large compilation benchmarks would be nice — the ideal stress test is probably something in C++ that instantiates a lot of function templates.

This is finicky, but: the FooBitfields structs are designed to be <= 64 bits, not just a 32-bit bitfield. Currently, CompoundStmt puts the leading brace location in there just to save some space on 32-bit hosts, but it seems to me that, if we're going to put a 32-bit count somewhere, it'd be nice to put it in the CompoundStmtBitfields and move the leading brace location out to the main class. I know, it's finicky.

[sepavloff]

Move NumStmts back to CompoundStmtBitfields

[sepavloff]

In D125635#3514607, @rjmccall wrote:

On the one hand, I'm not sure 8M statements in a block — or 1M, for that matter — is an unreasonable implementation limit. On the other hand, this patch looks like it only changes overall memory use on 32-bit hosts, because CompoundStmt has a 32-bit field prior to its pointer-aligned trailing storage. Can you check how memory use actually changes? Ideally you'd be able to test it on a 32-bit host, but if you don't have one, just knowing the raw count of allocated CompoundStmt objects for a few large compilation benchmarks would be nice — the ideal stress test is probably something in C++ that instantiates a lot of function templates.

I used the source file:

template<int K, int S>
struct F {
  static constexpr int func() {
    const int B = 4*(S-1);
    return F<(K+(0<<B)),S-1>::func() +
    F<(K+(1<<B)),S-1>::func() +
    F<(K+(2<<B)),S-1>::func() +
    F<(K+(3<<B)),S-1>::func() +
    F<(K+(4<<B)),S-1>::func() +
    F<(K+(5<<B)),S-1>::func() +
    F<(K+(6<<B)),S-1>::func() +
    F<(K+(7<<B)),S-1>::func() +
    F<(K+(8<<B)),S-1>::func() +
    F<(K+(9<<B)),S-1>::func() +
    F<(K+(10<<B)),S-1>::func() +
    F<(K+(11<<B)),S-1>::func() +
    F<(K+(12<<B)),S-1>::func() +
    F<(K+(13<<B)),S-1>::func() +
    F<(K+(14<<B)),S-1>::func() +
    F<(K+(15<<B)),S-1>::func() + 1;
  }
};

template<int K>
struct F<K, 0> {
  static constexpr int func() { return 1; }
};

template<int K>
struct F<K, 5> {
  static constexpr int func() {
    const int S = 5;
    const int B = 4*(S-1);
    return F<(K+(0<<B)),S-1>::func() +
    F<(K+(1<<B)),S-1>::func() +
    F<(K+(2<<B)),S-1>::func() +
    F<(K+(3<<B)),S-1>::func() +
    F<(K+(4<<B)),S-1>::func() +
    F<(K+(5<<B)),S-1>::func() +
    F<(K+(6<<B)),S-1>::func() + 1;
  }
};

int V = F<0, 5>::func();

and executed compiler with command:

clang -c -emit-llvm -fproc-stat-report -Xclang -disable-llvm-passes stress.cpp

Changing the second template parameter in the statement:

int V = F<0, 5>::func();

is used to set the number of functions.

The results are in the table, 3 samples for original and 3 for patched:

N functions	O1	O2	O3	P1	P2	P3
273	57596	56664	57228	57408	56972	58024
4369	63040	63200	63000	63208	63052	63720
69905	156752	156564	157436	156516	157044	157008
489336	788404	789864	790324	789072	789192	789528

The used host was 32-bit:

$ uname -a
Linux debian 5.10.0-14-686-pae #1 SMP Debian 5.10.113-1 (2022-04-29) i686 GNU/Linux

The change in numbers for original and patched versions is of the same order as variance between samples. It looks like process memory consumption is not a reliable way of the measurement in this case.

This is finicky, but: the FooBitfields structs are designed to be <= 64 bits, not just a 32-bit bitfield. Currently, CompoundStmt puts the leading brace location in there just to save some space on 32-bit hosts, but it seems to me that, if we're going to put a 32-bit count somewhere, it'd be nice to put it in the CompoundStmtBitfields and move the leading brace location out to the main class. I know, it's finicky.

It improves readability, both location are specified in the same place. Also, if SourceLocation becomes 64-bit it cannot be in CompoundStmtBitfields anyway.

[rjmccall]

Thanks, LGTM.

[sepavloff]

Thanks!

clang/include/clang/AST/Stmt.h

protected:
   };
 
   class CompoundStmtBitfields {
     friend class ASTStmtReader;
     friend class CompoundStmt;
 
     unsigned : NumStmtBits;
 
-    unsigned NumStmts : 32 - NumStmtBits;
-
-    /// The location of the opening "{".
-    SourceLocation LBraceLoc;
+    unsigned NumStmts;
   };
 
   class LabelStmtBitfields {
     friend class LabelStmt;
 
     unsigned : NumStmtBits;
 
     SourceLocation IdentLoc;

 };
 
 /// CompoundStmt - This represents a group of statements like { stmt stmt }.
 class CompoundStmt final : public Stmt,
                            private llvm::TrailingObjects<CompoundStmt, Stmt *> {
   friend class ASTStmtReader;
   friend TrailingObjects;
 
-  /// The location of the closing "}". LBraceLoc is stored in CompoundStmtBits.
+  /// The location of the opening "{".
+  SourceLocation LBraceLoc;
+
+  /// The location of the closing "}".
   SourceLocation RBraceLoc;
 
   CompoundStmt(ArrayRef<Stmt *> Stmts, SourceLocation LB, SourceLocation RB);
   explicit CompoundStmt(EmptyShell Empty) : Stmt(CompoundStmtClass, Empty) {}
 
   void setStmts(ArrayRef<Stmt *> Stmts);
 
 public:
   static CompoundStmt *Create(const ASTContext &C, ArrayRef<Stmt *> Stmts,
                               SourceLocation LB, SourceLocation RB);
 
   // Build an empty compound statement with a location.
   explicit CompoundStmt(SourceLocation Loc)
-      : Stmt(CompoundStmtClass), RBraceLoc(Loc) {
+      : Stmt(CompoundStmtClass), LBraceLoc(Loc), RBraceLoc(Loc) {
     CompoundStmtBits.NumStmts = 0;
-    CompoundStmtBits.LBraceLoc = Loc;
   }
 
   // Build an empty compound statement.
   static CompoundStmt *CreateEmpty(const ASTContext &C, unsigned NumStmts);
 
   bool body_empty() const { return CompoundStmtBits.NumStmts == 0; }
   unsigned size() const { return CompoundStmtBits.NumStmts; }
 
Stmt *getStmtExprResult() {
     }
     return body_back();
   }
 
   const Stmt *getStmtExprResult() const {
     return const_cast<CompoundStmt *>(this)->getStmtExprResult();
   }
 
-  SourceLocation getBeginLoc() const { return CompoundStmtBits.LBraceLoc; }
+  SourceLocation getBeginLoc() const { return LBraceLoc; }
   SourceLocation getEndLoc() const { return RBraceLoc; }
 
-  SourceLocation getLBracLoc() const { return CompoundStmtBits.LBraceLoc; }
+  SourceLocation getLBracLoc() const { return LBraceLoc; }
   SourceLocation getRBracLoc() const { return RBraceLoc; }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CompoundStmtClass;
   }
 
   // Iterators
   child_range children() { return child_range(body_begin(), body_end()); }

clang/lib/AST/Stmt.cpp

 }
 
 int64_t Stmt::getID(const ASTContext &Context) const {
   return Context.getAllocator().identifyKnownAlignedObject<Stmt>(this);
 }
 
 CompoundStmt::CompoundStmt(ArrayRef<Stmt *> Stmts, SourceLocation LB,
                            SourceLocation RB)
-    : Stmt(CompoundStmtClass), RBraceLoc(RB) {
+    : Stmt(CompoundStmtClass), LBraceLoc(LB), RBraceLoc(RB) {
   CompoundStmtBits.NumStmts = Stmts.size();
   setStmts(Stmts);
-  CompoundStmtBits.LBraceLoc = LB;
 }
 
 void CompoundStmt::setStmts(ArrayRef<Stmt *> Stmts) {
   assert(CompoundStmtBits.NumStmts == Stmts.size() &&
          "NumStmts doesn't fit in bits of CompoundStmtBits.NumStmts!");
 
   std::copy(Stmts.begin(), Stmts.end(), body_begin());
 }

clang/lib/Serialization/ASTReaderStmt.cpp

 
 void ASTStmtReader::VisitCompoundStmt(CompoundStmt *S) {
   VisitStmt(S);
   SmallVector<Stmt *, 16> Stmts;
   unsigned NumStmts = Record.readInt();
   while (NumStmts--)
     Stmts.push_back(Record.readSubStmt());
   S->setStmts(Stmts);
-  S->CompoundStmtBits.LBraceLoc = readSourceLocation();
+  S->LBraceLoc = readSourceLocation();
   S->RBraceLoc = readSourceLocation();
 }
 
 void ASTStmtReader::VisitSwitchCase(SwitchCase *S) {
   VisitStmt(S);
   Record.recordSwitchCaseID(S, Record.readInt());
   S->setKeywordLoc(readSourceLocation());
   S->setColonLoc(readSourceLocation());