<div dir="ltr"><div>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.</div><div><br></div><div>I'll summarize the capabilities of the 3 approaches in the following (kinda) table, using the following columns <b>[ Current LLVM | Clang | New RFC ]</b> :</div><div><div><ul><li><b>[_|X|X] static assert </b>- 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)</li><ul><li><b>[_|_|X] automatic static assert </b>- is adding the static assert needs to be manually or is it done automatically with the declaration of the new bitfield.</li></ul><li><b>[_|_|X] runtime assert</b> - that a new value set, fits into the the bitfield (without truncation).</li><li><b>[_|_|X] typed</b> - 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.</li><li><b>[X|_|X] declare in actual class</b> - as opposed to one of the base classes.</li><li><b>[_|_|X] declare (a bitfield) in a single line</b> - as opposed to the need to declare helpers or somekind, like `enum` (manually).</li><li><b>[_|_|X] clean bitfields</b> - without exposing a bit manipulation `enum`.</li><li><b>[_|_|X] automatic inheritance of unused bits</b> - no need to get offset from super (manually).</li><li><b>[_|_|X] automatic calculation of unused bits</b> - 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).</li><li><b>[_|_|X] implicit reference to superclass</b> - as opposed to the need to use the base class' info explicitly.</li><li><b>[_|_|X] no need to know anything about any of the base classes</b>.</li></ul><div>I think the table speaks for itself.</div></div><div><br></div><div>Craig, regarding the `getSubclassDataFromInstruction()`, it still does not turn the tides of the table, above, into the current implementation.</div><div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 27, 2019 at 8:57 PM Craig Topper <<a href="mailto:craig.topper@gmail.com">craig.topper@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"><div dir="ltr">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.<div><br clear="all"><div><div dir="ltr">~Craig</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 27, 2019 at 9:35 AM Bruno Ricci via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</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>
On 26/12/2019 20:53, Ehud Katz via llvm-dev wrote:<br>
> I've tested it on MSVC, gcc, clang and icc.<br>
> <br>
> 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.<br>
<br>
I spent some time packing various bits of data into bit-fields in clang. My experience<br>
is that the solution in clang actually works just fine. The Stmt hierarchy has a huge<br>
number of bit-field classes (I count more than 40! [1]), but because the offsets are<br>
relative adding a bit means only adding one to an enum value a few lines below<br>
(a static_assert then checks that the union is not too large).<br>
<br>
[1] <a href="https://github.com/llvm/llvm-project/blob/f20fc65887e2085332d111ee0b05736794423c72/clang/include/clang/AST/Stmt.h#L948" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/blob/f20fc65887e2085332d111ee0b05736794423c72/clang/include/clang/AST/Stmt.h#L948</a><br>
<br>
Bruno Ricci<br>
<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>
</blockquote></div>