[PATCH] Pack MCSymbol flags and common alignment
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Jul 1 12:32:33 PDT 2015
> 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?
LGTM either way, just watch the bots if you leave out the explicit
definition.
> + unsigned CommonAlignLog2 : NumCommonAlignmentBits;
> +
> /// Index field, for use by the object file implementation.
> mutable uint32_t Index = 0;
>
More information about the llvm-commits
mailing list