[PATCH] Pack MCSymbol flags and common alignment

[Pete Cooper]

> On Jul 1, 2015, at 12:32 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2015-Jul-01, at 12:18, Pete Cooper <peter_cooper at apple.com> wrote:
>> 
>>> 
>>> On Jul 1, 2015, at 12:15 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>>> 
>>>> On 2015-Jul-01, at 12:06, Pete Cooper <peter_cooper at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Jul 1, 2015, at 12:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>> 
>>>>> 
>>>>>> On 2015-Jun-30, at 14:30, Pete Cooper <peter_cooper at apple.com> wrote:
>>>>>> 
>>>>>> Here’s a new version of the patch for using 5-bits for alignment.  Please review when you get a chance.
>>>>>> 
>>>>>> Last one after this will just be to make flags 16-bits and move it to before Index.
>>>>>> 
>>>>>> Thanks,
>>>>>> Pete
>>>>>> 
>>>>>> <mcsymbol-packing.patch>
>>>>> 
>>>>>> diff --git a/include/llvm/MC/MCSymbol.h b/include/llvm/MC/MCSymbol.h
>>>>>> index 48b50d9..cb872be 100644
>>>>>> --- a/include/llvm/MC/MCSymbol.h
>>>>>> +++ b/include/llvm/MC/MCSymbol.h
>>>>>> @@ -109,6 +109,14 @@ protected:
>>>>>> /// extension and achieve better bitpacking with MSVC.
>>>>>> unsigned SymbolContents : 2;
>>>>>> 
>>>>>> +  /// The alignment of the symbol, if it is 'common', or -1.
>>>>>> +  ///
>>>>>> +  /// The alignment is stored as log2(align) + 1.  This allows all values from
>>>>>> +  /// 0 to 2^31 to be stored which is every power of 2 representable by an
>>>>>> +  /// unsigned.
>>>>>> +  const unsigned NumCommonAlignmentBits = 5;
>>>>> 
>>>>> I think you meant to make this static, right?  Otherwise it costs 4
>>>>> bytes.
>>>> Good point.  I did mean that.  I can make that change before I commit.
>>>>> 
>>>>>> +  unsigned CommonAlignLog2 : 5;
>>>>> 
>>>>> Can you use `NumCommonAlignmentBits` here?
>>>> Unfortunately that doesn’t work.  I would love to be able to use constant expressions for bitfield sizes. Keeping the lines adjacent is the only thing I can think to do for now.
>>> 
>>> This compiles cleanly for me.  Is it an MSVC issue?
>>> --
>>> $ clang++ -std=c++11 t.cpp  -c -Wall -Wextra
>>> $ cat t.cpp 
>>> struct T {
>>> static const int X = 4;
>>> int Bits : X;
>>> };
>> You’re right.  Thats compiling for me too.  I don’t know what I was thinking, but i was sure i’d had a problem with this compiling with clang.  I did the same in Value.h for example:
>> 
>>  static const unsigned NumUserOperandsBits = 29;
>>  unsigned NumUserOperands : 29;
>> 
>> How does this patch look given the changes you found?
>> Pete
>> 
>> <mcsymbol-packing.patch>
> 
>> diff --git a/include/llvm/MC/MCSymbol.h b/include/llvm/MC/MCSymbol.h
>> index 48b50d9..e52a346 100644
>> --- a/include/llvm/MC/MCSymbol.h
>> +++ b/include/llvm/MC/MCSymbol.h
>> @@ -109,6 +109,14 @@ protected:
>>   /// extension and achieve better bitpacking with MSVC.
>>   unsigned SymbolContents : 2;
>> 
>> +  /// The alignment of the symbol, if it is 'common', or -1.
>> +  ///
>> +  /// The alignment is stored as log2(align) + 1.  This allows all values from
>> +  /// 0 to 2^31 to be stored which is every power of 2 representable by an
>> +  /// unsigned.
>> +  static const unsigned NumCommonAlignmentBits = 5;
> 
> IIRC, you need something like this in MCSymbol.cpp for -O0 builds:
> 
>    const unsigned MCSymbol::NumCommonAlignmentBits;
> 
> to avoid linker errors.  However, maybe none of our supported
> compilers require that anymore?
You’re right (in that we do this elsewhere, and I don’t want to potentially break bots).  I’ve added that too.  
> 
> LGTM either way, just watch the bots if you leave out the explicit
> definition.
Thanks.  r241189.

Pete
> 
>> +  unsigned CommonAlignLog2 : NumCommonAlignmentBits;
>> +
>>   /// Index field, for use by the object file implementation.
>>   mutable uint32_t Index = 0;

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150701/58819cea/attachment.html>