[clang][NFC] Increase NumStmtBits by 2 as we are approaching the limit

[ziqingluo-90]

We have already hit the limit of NumStmtBits downstream after 010d011, which adds 4 new StmtNodes.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

We have already hit the limit of NumStmtBits downstream after 010d011, which adds 4 new StmtNodes.

---

Full diff: https://github.com/llvm/llvm-project/pull/120341.diff

1 Files Affected:

• (modified) clang/include/clang/AST/Stmt.h (+15-4)

diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 83fafbabb1d460..4d02b122c5e858 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -109,6 +109,18 @@ class alignas(void *) Stmt {
 
   //===--- Statement bitfields classes ---===//
 
+  enum { NumStmtBits = 10 };
+
+#define STMT(CLASS, PARENT)
+#define STMT_RANGE(BASE, FIRST, LAST)
+#define LAST_STMT_RANGE(BASE, FIRST, LAST)                                     \
+  static_assert(                                                               \
+      StmtClass::LAST##Class < (1 << NumStmtBits),                             \
+      "The number of 'StmtClass'es is strictly bounded under two to "          \
+      "the power of 'NumStmtBits'");
+#define ABSTRACT_STMT(STMT)
+#include "clang/AST/StmtNodes.inc"
+
   class StmtBitfields {
     friend class ASTStmtReader;
     friend class ASTStmtWriter;
@@ -116,9 +128,8 @@ class alignas(void *) Stmt {
 
     /// The statement class.
     LLVM_PREFERRED_TYPE(StmtClass)
-    unsigned sClass : 8;
+    unsigned sClass : NumStmtBits;
   };
-  enum { NumStmtBits = 8 };
 
   class NullStmtBitfields {
     friend class ASTStmtReader;
@@ -564,8 +575,8 @@ class alignas(void *) Stmt {
     /// True if the call expression is a must-elide call to a coroutine.
     unsigned IsCoroElideSafe : 1;
 
-    /// Padding used to align OffsetToTrailingObjects to a byte multiple.
-    unsigned : 24 - 4 - NumExprBits;
+    static_assert(NumExprBits == 20,
+                  "No extra padding needed when NumExprBits is exactly 20.");
 
     /// The offset in bytes from the this pointer to the start of the
     /// trailing objects belonging to CallExpr. Intentionally byte sized

[rjmccall]

Let's just increase to 9 bits.

Have you checked whether the size of Stmt or Expr changes when you do this?

[erichkeane]

Let's just increase to 9 bits.

Have you checked whether the size of Stmt or Expr changes when you do this?

I have these concerns as well, this ends up affecting the size of a lot of things....

That said, StmtNodes.td already has 267 items in it (as far as I can tell, just a wc -l on appearances of 'def'). So I don't know how we're getting away with this already.

Can you confirm how many are present currently?

I also think 9 should be plenty, 512 should cover us for many many years.

[erichkeane]

Let's just increase to 9 bits.
Have you checked whether the size of Stmt or Expr changes when you do this?

I have these concerns as well, this ends up affecting the size of a lot of things....

That said, StmtNodes.td already has 267 items in it (as far as I can tell, just a wc -l on appearances of 'def'). So I don't know how we're getting away with this already.

Can you confirm how many are present currently?

I also think 9 should be plenty, 512 should cover us for many many years.

I THINK I just tracked down that our current max is ~249, and I intend to add a few more for OpenACC, so this would cause us to exceed the limit, even without a downstream. Additionally, Reflection is likely to add a few, as will contracts, so even without OpenACC, we'd continue to exceed the limit.

I'd like to see some level of analysis of what we're paying for this increase, and whether we can 'get it back' elsewhere (some of our bitfields have padding we could perhaps steal, or some too-large-but-nothing-better-needs-the-bits fields), so we should make sure we minimize the sizeof our important things.

Perhaps some level of "print sizeof every AST node before and after" could be done, and compared?

[jroelofs]

Let's just increase to 9 bits.
Have you checked whether the size of Stmt or Expr changes when you do this?

I have these concerns as well, this ends up affecting the size of a lot of things....
That said, StmtNodes.td already has 267 items in it (as far as I can tell, just a wc -l on appearances of 'def'). So I don't know how we're getting away with this already.
Can you confirm how many are present currently?
I also think 9 should be plenty, 512 should cover us for many many years.

I THINK I just tracked down that our current max is ~249, and I intend to add a few more for OpenACC, so this would cause us to exceed the limit, even without a downstream.

quick way to find those cases:

diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 59c3284b422b..fb3862d42d04 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1440,6 +1440,7 @@ public:
                   "changing bitfields changed sizeof(Stmt)");
     static_assert(sizeof(*this) % alignof(void *) == 0,
                   "Insufficient alignment!");
+    assert(llvm::isInt<NumStmtBits>(SC));
     StmtBits.sClass = SC;
     if (StatisticsEnabled) Stmt::addStmtClass(SC);
   }

[ziqingluo-90]

Let's just increase to 9 bits.

Have you checked whether the size of Stmt or Expr changes when you do this?

@rjmccall I've changed it to 9 bits and checked that Stmt and Expr are still 8 bytes and 16 bytes resp..

[erichkeane]

Let's just increase to 9 bits.
Have you checked whether the size of Stmt or Expr changes when you do this?

@rjmccall I've changed it to 9 bits and checked that Stmt and Expr are still 8 bytes and 16 bytes resp..

I think checking just those two isn't sufficient, as this is used as a 'base' type for a few other types. We probably need to ensure that other types that use bitfields don't have this problem as well. Basically any of the other bitfield types here that implicitly use StmtBitfields in the mask. NullStmt, CompoundStmt, and LabelStmt, (and every other use of NumStmtBits) are all ones that need checking.

[rjmccall]

Let's just increase to 9 bits.
Have you checked whether the size of Stmt or Expr changes when you do this?

@rjmccall I've changed it to 9 bits and checked that Stmt and Expr are still 8 bytes and 16 bytes resp..

I think checking just those two isn't sufficient, as this is used as a 'base' type for a few other types. We probably need to ensure that other types that use bitfields don't have this problem as well. Basically any of the other bitfield types here that implicitly use StmtBitfields in the mask. NullStmt, CompoundStmt, and LabelStmt, (and every other use of NumStmtBits) are all ones that need checking.

The way this works is that all of these class-specific bitfields are union'ed into a single field in Stmt, so actually the layout of the subclasses won't change as long as the size of Stmt doesn't.

[erichkeane]

Let's just increase to 9 bits.
Have you checked whether the size of Stmt or Expr changes when you do this?

@rjmccall I've changed it to 9 bits and checked that Stmt and Expr are still 8 bytes and 16 bytes resp..

I think checking just those two isn't sufficient, as this is used as a 'base' type for a few other types. We probably need to ensure that other types that use bitfields don't have this problem as well. Basically any of the other bitfield types here that implicitly use StmtBitfields in the mask. NullStmt, CompoundStmt, and LabelStmt, (and every other use of NumStmtBits) are all ones that need checking.

The way this works is that all of these class-specific bitfields are union'ed into a single field in Stmt, so actually the layout of the subclasses won't change as long as the size of Stmt doesn't.

OH! I see! I misremembered how this works. Disregard the above then.

[erichkeane]

Please wait a little while to give the others a chance to approve this version before committing.

[rjmccall]

LGTM, but yeah, please give some time for the other early reviewers to sign off as well.

[nikic]

FYI it looks like this has a fairly large impact on compile-time, esp at O0 with a clang host compiler (https://llvm-compile-time-tracker.com/compare.php?from=1fcb6a9754a8db057e18f629cb90011b638901e7&to=4797437463e63ee289a1ff1904cfb7b2fe6cb4c2&stat=instructions%3Au).

I wonder whether this is due to some increase in structure sizes, or because the choice of a non power of two bitwidth makes the accesses to the field inefficient.

[nikic]

Not sure how much of the (clang bootstrap) build time overhead comes from this, but this seems like the kind of assertion that should be in a source file, not a header.

[rjmccall]

FYI it looks like this has a fairly large impact on compile-time, esp at O0 with a clang host compiler (https://llvm-compile-time-tracker.com/compare.php?from=1fcb6a9754a8db057e18f629cb90011b638901e7&to=4797437463e63ee289a1ff1904cfb7b2fe6cb4c2&stat=instructions%3Au).

I wonder whether this is due to some increase in structure sizes, or because the choice of a non power of two bitwidth makes the accesses to the field inefficient.

It probably is the bit-width not matching a native integer size. If we want to get that back, we'll need to Huffman-encode StmtKind — not actual Huffman encoding, of course, but making a BasicStmtKind that includes our favorite 255 node kinds ("common nodes") and then making the last basic kind cover all the uncommon kinds with some secondary extended-kind field (probably also 8-bit?). This would generalize in the future if necessary, in that we could carve out more values out of BasicStmtKind if we needed more than 511 nodes.

Computing the actual StmtKind would become more expensive, essentially this:

auto kind = node->basicKind;   // 8-bit load
if (kind == 255)
  kind += node->extendedKind;  // 8-bit load

But isa/dyn_cast checks for common nodes would only need to load the basic kind. And we could make sure that StmtVisitor et al. also privilege common nodes, like by switching on the basic kind and then switching on the extended kind in case 255 instead of computing the full StmtKind up front and then doing a single switch.

We should be able to make ClangASTNodesEmitter generate all of the code for us. The emitter would require uncommon nodes to be marked in StmtNodes.td and check that there aren't too many common nodes. We'd probably want it to start synthesizing classof implementations, too, but that's pretty straightforward and would remove some boilerplate from the AST headers. (The compiler would still see it, so it's not a compile-time win for processing the header, just a programmer burden removed.)