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

[Davide Italiano via llvm-commits]

On Wed, Apr 13, 2016 at 10:46 AM, Duncan P. N. Exon Smith via
llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
>> 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.)
>

I don't mind changing it.

>> +
>> +  unsigned Flags:27;
>
> What do we actually use right now?  Should this just be an
> `unsigned short` or something (outside the bitfield)?

We actually use 15 flags so we're on the edge if  we want to use a
unsigned char.
Given that moving it outside of the bitfiled doesn't buy us anything,
I'd like to keep it as is (and allow more space in case more flags are
added).

>
>> +
>> +  // 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>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare