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

[Robinson, Paul via llvm-commits]

> -----Original Message-----
> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
> Sent: Wednesday, April 13, 2016 10:46 AM
> To: reviews+D18918+public+652e7f8ee94138cf at reviews.llvm.org
> Cc: dccitaliano at gmail.com; Adrian Prantl; Eric Christopher; Robinson,
> Paul; llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D18918: [DebugInfo] Try to make class memory layout
> more efficient
> 
> 
> > 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?

My comment has been addressed, thanks for checking.
--paulr

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