[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D50630#1197795, @riccibruno wrote:

> @rjmccall
>
> I would argue that we should make these bit-fields take 8 bytes for the following reasons:
>
> 1. On 64 bits archs, this is free since we have already a little less than 8 bytes of padding here, assuming Type keep its 18 bits.


First, `Type` is not 16-byte aligned as far as the compiler is concerned; it is merely
dynamically aligned to a 16-byte boundary when allocated.  You actually verified this
when you did your experiment of printing out the sizes of various `Type` subclasses,
because the value of `sizeof` is always rounded up to the alignment, meaning that a
size of 40 would not be legal if `Type` were actually 16-byte-aligned.

Because `Type` is only 4/8-byte aligned, its only tail padding is what's needed to
fill up a pointer after these bit-fields, which is supposed to be 4 bytes but is
instead apparently 0 bytes because we overflowed one of the bit-fields.

Not officially aligning `Type` to 16 bytes is fairly important for memory efficiency
even on 64-bit hosts because many of our types use tail-allocated storage, which
naturally starts at `sizeof(T)` bytes from the address point; if we formally aligned
`Type` subclasses to 16 bytes, we'd often waste a pointer of that storage.

Second, tail padding of base classes is not necessarily wasted under the Itanium C++ ABI.
Itanium will start placing sub-objects in the tail-padding of the last non-empty base class
as long as that base is POD under the rules of C++03; `Type` is not POD because it has a
user-provided constructor.

Looking at `Type.h`, we don't seem to be taking advantage of this as much as I thought,
at least in the `Type` hierarchy (maybe we do in the `Stmt` or `Decl` hierarchies).  We
have a lot of subclasses that have 32 bits of miscellaneous storage but either (1) don't
order their fields correctly to allow subclasses to fit in or (2) also subclass `FoldingSetNode`,
which breaks up the optimization.

Since we do care primarily about 64-bit hosts, I'm leaning towards agreeing that we should
just accept that we have 64 bits of storage here, 46 of which are available to subclasses.
If you're interested in optimizing this, it would be nice if you could go through all the
subclasses looking for 32-bit chunks that could be profitably moved up into `Type`.

> 2. On 32 bits archs, this a priori increase the size of each Type by 4 bytes. However, types are aligned to 16 bytes because of the fast CVR qualifiers. Doing an fsyntax-only on all of Boost with -print-stats I get that by far the most commonly used Types have size 40 or 48 (on a 64 bits arch). That is 5 or 6 pointers. This would be 20 or 24 bytes on 32 bits archs if we assume that every member of the most commonly used types are pointers and that we shrink the bitfields to 32 bits. But since Types are aligned to 16 bytes we do not gain anything.

Types aren't the only thing allocated in the ASTContext, so allocating a 16-byte-aligned
chunk with a non-multiple-of-16 size doesn't mean that any difference between the size
and the next 16-byte boundary is necessarily wasted.  But you're probably right that the
difference between `Type` being 12 and 16 bytes is probably not a big deal.

John.


Repository:
  rC Clang

https://reviews.llvm.org/D50630