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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191224/fa3bcd07/attachment.html>
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/47069abb/attachment.html>
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>