Page MenuHomePhabricator

Make CompoundStmtBitfields::NumStmts not a bit-field
ClosedPublic

Authored by sepavloff on May 15 2022, 10:10 AM.

Details

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.

Diff Detail

Event Timeline

sepavloff created this revision.May 15 2022, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 10:10 AM
sepavloff requested review of this revision.May 15 2022, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 10:10 AM

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 updated this revision to Diff 430613.May 19 2022, 2:27 AM

Move NumStmts back to CompoundStmtBitfields

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 functionsO1O2O3P1P2P3
273575965666457228574085697258024
4369630406320063000632086305263720
69905156752156564157436156516157044157008
489336788404789864790324789072789192789528

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.

sepavloff retitled this revision from Move NumStmts from CompoundStmtBitfields to fields of CompoundStmt to Make CompoundStmtBitfields::NumStmts not a bit-field.May 19 2022, 8:06 AM
sepavloff edited the summary of this revision. (Show Details)
rjmccall accepted this revision.May 19 2022, 7:42 PM

Thanks, LGTM.

This revision is now accepted and ready to land.May 19 2022, 7:42 PM
This revision was automatically updated to reflect the committed changes.