[PATCH] D18918: [DebugInfo] Try to make class memory layout more efficient

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Apr-13, at 10:03, Davide Italiano <dccitaliano at gmail.com> wrote:
> 
> davide updated this revision to Diff 53584.
> davide added a comment.
> 
> Done. Any other comments?

This LGTM if you address a couple of nitpicks below.

Adrian and Paul, anything else from you?

> http://reviews.llvm.org/D18918
> 
> Files:
>  include/llvm/IR/DebugInfoMetadata.h
>  unittests/IR/MetadataTest.cpp
> 
> Index: unittests/IR/MetadataTest.cpp
> ===================================================================
> --- unittests/IR/MetadataTest.cpp
> +++ unittests/IR/MetadataTest.cpp
> @@ -1413,7 +1413,7 @@
>   bool IsDefinition = true;
>   unsigned ScopeLine = 3;
>   DITypeRef ContainingType = getCompositeType();
> -  unsigned Virtuality = 4;
> +  unsigned Virtuality = 2;
>   unsigned VirtualIndex = 5;
>   unsigned Flags = 6;
>   bool IsOptimized = false;
> Index: include/llvm/IR/DebugInfoMetadata.h
> ===================================================================
> --- include/llvm/IR/DebugInfoMetadata.h
> +++ include/llvm/IR/DebugInfoMetadata.h
> @@ -1229,22 +1229,34 @@
> 
>   unsigned Line;
>   unsigned ScopeLine;
> -  unsigned Virtuality;
>   unsigned VirtualIndex;
> -  unsigned Flags;
> -  bool IsLocalToUnit;
> -  bool IsDefinition;
> -  bool IsOptimized;
> +
> +  // Virtuality can only assume three values, so we can pack
> +  // in 2 bits (none/pure/pure_virtual).
> +  unsigned Virtuality:2;

I'm accustomed to seeing spaces before/after the `:` and I think
they'd make it easier to read.  What does clang-format-diff.py
say about this one?  (If it's happy without spaces then I'll
adjust.)

> +
> +  unsigned Flags:27;

What do we actually use right now?  Should this just be an
`unsigned short` or something (outside the bitfield)?

> +
> +  // These are boolean flags so one bit is enough.
> +  // MSVC starts a new container field every time the base
> +  // type changes so we can't use 'bool' to ensure these bits
> +  // are packed.
> +  unsigned IsLocalToUnit:1;
> +  unsigned IsDefinition:1;
> +  unsigned IsOptimized:1;
> 
>   DISubprogram(LLVMContext &C, StorageType Storage, unsigned Line,
>                unsigned ScopeLine, unsigned Virtuality, unsigned VirtualIndex,
>                unsigned Flags, bool IsLocalToUnit, bool IsDefinition,
>                bool IsOptimized, ArrayRef<Metadata *> Ops)
>       : DILocalScope(C, DISubprogramKind, Storage, dwarf::DW_TAG_subprogram,
>                      Ops),
> -        Line(Line), ScopeLine(ScopeLine), Virtuality(Virtuality),
> -        VirtualIndex(VirtualIndex), Flags(Flags), IsLocalToUnit(IsLocalToUnit),
> -        IsDefinition(IsDefinition), IsOptimized(IsOptimized) {}
> +        Line(Line), ScopeLine(ScopeLine), VirtualIndex(VirtualIndex),
> +        Virtuality(Virtuality), Flags(Flags), IsLocalToUnit(IsLocalToUnit),
> +        IsDefinition(IsDefinition), IsOptimized(IsOptimized) {
> +    static_assert(dwarf::DW_VIRTUALITY_max < 4, "Virtuality out of range");
> +    assert(Virtuality < 4 && "Virtuality out of range");

Please add an assertion for `Flags` (whatever the size).

> +  }
>   ~DISubprogram() = default;
> 
>   static DISubprogram *
> 
> 
> <D18918.53584.patch>