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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In https://reviews.llvm.org/D50630#1197930, @rjmccall wrote:

> 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.


Right,

> 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.

I just realized that something like this was happening when playing with various classes on godbolt.
I should go read the Itanium spec ! However as you say the condition for this happening are
not necessarily satisfied and moreover other ABIs might not do this packing (but you almost
certainly are more familiar with this than me)

> 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.

I actually did exactly this. My approach was to extend what is already done,
that is add nested classes SomethingBitfields in Type and add an instance of
each to the anonymous union. The classes which I have found so far benefiting
from this are FunctionProtoType, TemplateSpecializationType, PackExpansionType,
DependentTemplateSpecializationType and SubstTemplateTypeParmPackType.

For example the patch dealing with TemplateSpecializationType is
here https://reviews.llvm.org/D50643.


Repository:
  rC Clang

https://reviews.llvm.org/D50630