<div dir="ltr">The solution can support most (if not all) of the features you have mentioned. It can always be improved if needed, but for now, it is a great, neat, solution for the classes in the LLVM IR library.<div><br><div>Just for an example: using the new implementation, I have managed to move the SSID field into the Subclass Data of the Instructions, and in turn reduce the size of those instructions by 8 bytes.</div><div><br><div>I have opened a new review for the RFC: </div><div><a href="https://reviews.llvm.org/D72068">https://reviews.llvm.org/D72068</a> </div></div></div><div><br></div><div>Please review it.</div><div>Thanks,</div><div>Ehud.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 30, 2019 at 3:23 PM Bruno Ricci <<a href="mailto:riccibrun@gmail.com">riccibrun@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
Do you have some code we can look at (even if it is in a nasty unpolished state, just mark it WIP<br>
and put it on Phab) ? It is hard to evaluate an alternative without the code. That said I think<br>
that the table is a little bit one-sided. I have added some inline comments.<br>
<br>
On 30/12/2019 11:53, Ehud Katz wrote:<br>
> The solution in Clang is still very complicated and error prone. A lot of synchronization work (between different classes - at least in the same hierarchy) needs to be done manually.<br>
> <br>
> I'll summarize the capabilities of the 3 approaches in the following (kinda) table, using the following columns *[ Current LLVM | Clang | New RFC ]* :<br>
> <br>
> * *[_|X|X] static assert *- that the declared accumulated bitfields do not exceed the underlying subclass data size (note that int the New implementation it is automatically added on declaration)<br>
> o *[_|_|X] automatic static assert *- is adding the static assert needs to be manually or is it done automatically with the declaration of the new bitfield.<br>
<br>
This is not actually true. There is only a single static_assert for the size of<br>
the union in Stmt [1] and the same could be done for the union in DeclBase.<br>
<br>
> * *[_|_|X] runtime assert* - that a new value set, fits into the the bitfield (without truncation).<br>
<br>
This is true, I agree here that this is useful.<br>
<br>
> * *[_|_|X] typed* - as opposed to using a representative type (like `int`) and then cast to the actual required type (like `bool` or `enum`). Typed (ordinary) bitfields cannot be implemented correctly in MSVC, as the types of all the bitfields must be of the same type. Using typed bitfields also saves us the need to synchronize the use of `unsigned/signed int` with the actual type needed.<br>
> * *[X|_|X] declare in actual class* - as opposed to one of the base classes.<br>
<br>
True, but I am curious to see how you can avoid doing this in the base. The whole point<br>
of doing this bit-field thing is to save space by reusing the padding after the base<br>
subobject. Maybe with an aligned char array reinterpret_cast'ed by the derived classes<br>
as mentioned before? The trick is that we don't want to repeat the members from the<br>
base classes. Also we have to avoid UB. <br>
<br>
> * *[_|_|X] declare (a bitfield) in a single line* - as opposed to the need to declare helpers or somekind, like `enum` (manually)> * *[_|_|X] clean bitfields* - without exposing a bit manipulation `enum`.<br>
<br>
The enum is only used by the other bit-field classes. This is invisible to the class itself.<br>
No helpers are needed: just write SomeClassBits.SomeBit to refer to SomeBit in the<br>
bit-field of SomeClass.<br>
<br>
> * *[_|_|X] automatic inheritance of unused bits* - no need to get offset from super (manually).<br>
> * *[_|_|X] automatic calculation of unused bits* - changing a single bitfield doesn't require any other change, but the actual bitfield itself (as opposed to changing also the sum of the bit count used by the class, in an `enum` - for exmple).<br>
> * *[_|_|X] implicit reference to superclass* - as opposed to the need to use the base class' info explicitly.<br>
> * *[_|_|X] no need to know anything about any of the base classes*.<br>
<br>
I agree that forgetting to update the offset in the enum can be a source of error (but<br>
the enum is just a few lines below so it is hard to miss). Some things to keep in mind:<br>
<br>
- Some bit-fields are aligned to a byte boundary for (benchmarked) faster access [2]<br>
- Some bit-field classes have a hole which is used by derived classes [3]<br>
- Some bit-fields do not have a fixed size, just something "large enough".<br>
(this is not ideal and it would be better to have a well-defined limit<br>
which could then be used to trigger an error instead of just overflowing,<br>
that's on my TODO list...) [4]<br>
<br>
> <br>
> I think the table speaks for itself.<br>
> <br>
> Craig, regarding the `getSubclassDataFromInstruction()`, it still does not turn the tides of the table, above, into the current implementation.<br>
><br>
<br>
Cheers!<br>
<br>
[1] <a href="https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L1088" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L1088</a><br>
[2] <a href="https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L471" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L471</a><br>
[3] <a href="https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L849" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L849</a><br>
[4] <a href="https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L804" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/Stmt.h#L804</a><br>
<br>
Bruno Ricci<br>
</blockquote></div>