[PATCH] D125635: Move NumStmts from CompoundStmtBitfields to fields of CompoundStmt

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D125635#3514607 <https://reviews.llvm.org/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 <https://reviews.llvm.org/owners/package/1/>      | O2 <https://reviews.llvm.org/owners/package/2/>      | O3 <https://reviews.llvm.org/owners/package/3/>      | P1 <https://reviews.llvm.org/P1>      | P2 <https://reviews.llvm.org/P2>      | P3 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125635/new/

https://reviews.llvm.org/D125635