llvm dev - Dec 2019 - RFC: Refactor SubclassData

I've tested it on MSVC, gcc, clang and icc.

The solution in clang (in Decl.h) is not ideal (as you have said yourself).
The solution I offer, is using a union of fields of class BitField (this is
a new class that implements a bitfield of a specific type requested). With
this, the definition, of the required bitfields in the subclass data,
remains in the hands of the actual class needing them. And these are all
restricted hierarchically by the superclasses of that class.

On Thu, Dec 26, 2019 at 10:22 PM Reid Kleckner <rnk at google.com> wrote:
> First, please test your technique with MSVC on godbolt. If you change the
> type (bool a : 1; int b : 2;), it will not pack them into the same
bitfield.
>
> Second, do we really need macros to do this? See the techniques used in
> clang::VarDecl:
>
>
https://github.com/llvm/llvm-project/blob/f20fc65887e2085332d111ee0b05736794423c72/clang/include/clang/AST/Decl.h#L878
> As written with a union between all the bitfields, the base class ends up
> knowing everything about the derived classes, which is not ideal. But, you
> could imagine a version of this that uses aligned char storage so that
> subclasses could reinterpret_cast the subclass data memory and use it for
> their version of subclass data.
>
> On Mon, Dec 23, 2019 at 11:16 PM Ehud Katz via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hello devs,
>>
>> Recently I've been working on a bug that can probably be fixed with
the
>> addition of a simple flag to a class descendant of `llvm::Value`.
>> Without increasing the size of the class, I can just add this flag to
>> `llvm::Value::SubclassData`. But this is not an easy task!
>>
>> This is because the offsetes/sizes of the data stored in the
>> `SubclassData`, are hardcoded literals/enums.
>> If you change but a single bit in one of those offsets/sizes in a base
>> class, then all the classes derived, will have to be adjusted
accordingly -
>> which can be very tedious and error-prone.
>>
>> I offer a small set of macros which will take care of the whole
>> Subclass-Data.
>> It will work as follows:
>>
>> struct A {
>>   int SubclassData : 14;
>>
>>   DEFINE_SUBCLASS_DATA()
>> };
>>
>> struct B : A {
>>   BEGIN_SUBCLASS_DATA()
>>     ADD_SUBCLASS_BITFIELD(int,   5, B1) // A::SubclassData bits [0,5)
>> ADD_SUBCLASS_BITFIELD(bool,  1, B2) // A::SubclassData bits [5,6)
>> ADD_SUBCLASS_BITFIELD(short, 6, B3) // A::SubclassData bits [6,12)
>>  // ADD_SUBCLASS_BITFIELD(int,   6, B4) // A::SubclassData bits [12,18)
-
>> triggers a static_assert, as it exceeds the 14 bits in A::SubclassData
>>     END_SUBCLASS_DATA()
>> };
>>
>> struct C : B {
>>   BEGIN_SUBCLASS_DATA()
>>     ADD_SUBCLASS_BITFIELD(bool, 1, C1) // A::SubclassData bits [12,13)
>>     END_SUBCLASS_DATA()
>> };
>>
>>
>> I would appreciate your thoughts on the matter, before I submit a patch
>> for review.
>>
>> Cheers,
>> Ehud.
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191226/50f6ab52/attachment.html>

Hi,

On 26/12/2019 20:53, Ehud Katz via llvm-dev wrote:
> I've tested it on MSVC, gcc, clang and icc.
> 
> The solution in clang (in Decl.h) is not ideal (as you have said yourself).
The solution I offer, is using a union of fields of class BitField (this is a
new class that implements a bitfield of a specific type requested). With this,
the definition, of the required bitfields in the subclass data, remains in the
hands of the actual class needing them. And these are all restricted
hierarchically by the superclasses of that class.
I spent some time packing various bits of data into bit-fields in clang. My
experience
is that the solution in clang actually works just fine. The Stmt hierarchy has a
huge
number of bit-field classes (I count more than 40! [1]), but because the offsets
are
relative adding a bit means only adding one to an enum value a few lines below
(a static_assert then checks that the union is not too large).

[1]
https://github.com/llvm/llvm-project/blob/f20fc65887e2085332d111ee0b05736794423c72/clang/include/clang/AST/Stmt.h#L948

Bruno Ricci

Ehud, can you elaborate on which classes you're trying to change. I know
some of the classes already use methods
like getSubclassDataFromInstruction() to hide bits from the subclasses.
They could probably shift the data too.

~Craig


On Fri, Dec 27, 2019 at 9:35 AM Bruno Ricci via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> On 26/12/2019 20:53, Ehud Katz via llvm-dev wrote:
> > I've tested it on MSVC, gcc, clang and icc.
> >
> > The solution in clang (in Decl.h) is not ideal (as you have said
> yourself). The solution I offer, is using a union of fields of class
> BitField (this is a new class that implements a bitfield of a specific type
> requested). With this, the definition, of the required bitfields in the
> subclass data, remains in the hands of the actual class needing them. And
> these are all restricted hierarchically by the superclasses of that class.
>
> I spent some time packing various bits of data into bit-fields in clang.
> My experience
> is that the solution in clang actually works just fine. The Stmt hierarchy
> has a huge
> number of bit-field classes (I count more than 40! [1]), but because the
> offsets are
> relative adding a bit means only adding one to an enum value a few lines
> below
> (a static_assert then checks that the union is not too large).
>
> [1]
>
https://github.com/llvm/llvm-project/blob/f20fc65887e2085332d111ee0b05736794423c72/clang/include/clang/AST/Stmt.h#L948
>
> Bruno Ricci
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191227/2c2706fb/attachment.html>