<div dir="ltr">Documentation and code have been polished.<div>I'll go ahead and submit the version #3 of the design.</div><div><a href="https://reviews.llvm.org/D82454">https://reviews.llvm.org/D82454</a><br></div><div><br></div><div>Thank you to everyone who contributed!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 24, 2020 at 2:21 PM Guillaume Chatelet <<a href="mailto:gchatelet@google.com">gchatelet@google.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">Hi River,<div><br></div><div>I've to say I really like your suggestion.</div><div>I went ahead and implemented it as <a href="https://reviews.llvm.org/D82454" target="_blank">https://reviews.llvm.org/D82454</a></div><div><br></div><div>I'll wait a bit more for other people to chime in but overall it has my preference.</div><div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 23, 2020 at 9:51 PM River Riddle <<a href="mailto:riddleriver@gmail.com" target="_blank">riddleriver@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">Hi Guillaume,<div><br></div><div>Thanks for the RFC. I'm generally +1 on the concept. Making bit field manipulation easier seems like a good overall goal given its prevalence in LLVM.</div><div><br></div><div>As for the syntax, I tend to prefer that we don't pollute the namespace. Have you considered pushing the methods into the Bitfield class? Maybe something like:</div><div><br></div><div>``` </div><div> uint8_t Storage = 0;</div> using Amount = <span>Bitfield::Element</span><unsigned, 0, 4>;<br><div><br> Bitfield::set<Amount>(Storage, true);<br> unsigned Value = Bitfield::get<Amount>(Storage);<br> bool Cond = Bitfield::test<Amount>(Storage);<br>```</div><div><br></div><div>^ Seems good to me because it's clear what the API is doing, even without knowing what `Amount` is. WDYT?</div><div><br></div><div>-- River</div><div> </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 23, 2020 at 9:40 AM Guillaume Chatelet 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"><div dir="ltr"><div>The code is now fairly stable and well tested but the syntax is not yet final.</div><div>I would like to get some feedback from the community on alternative syntaxes that have been suggested to me.</div><div><br></div><div>First, the designed API defines three functions:</div><div> - get: unpacks the value from storage,</div><div> - set: packs the provided value and stores it in storage,</div><div><div> - test: efficiently returns whether the field is non zero.</div><div><br></div><div></div></div><div>Now, the three syntaxes:</div><div><br></div><div>1. Current API, purely functional<br>code: <a href="https://reviews.llvm.org/D81580" target="_blank">https://reviews.llvm.org/D81580</a><br><br> uint8_t Storage = 0;<br> using Amount = Bitfield<unsigned, 0, 4>;<br><br> setField<Amount>(Storage, true);<br> unsigned Value = getField<Amount>(Storage);<br> bool Cond = testField<Amount>(Storage);<br><br>2. A more Object Oriented API<br>code: <a href="https://reviews.llvm.org/D82058" target="_blank">https://reviews.llvm.org/D82058</a><br><br> uint8_t Storage = 0;<br> using Amount = Bitfield<unsigned, 0, 4>;<br><br> bitfield<Amount>(Storage) = true;<br> unsigned Value = bitfield<Amount>(Storage);<br> bool Cond = bitfield<Amount>(Storage).isSet();<br><br>3. In between<br>code: <a href="https://reviews.llvm.org/D82058#inline-754912" target="_blank">https://reviews.llvm.org/D82058#inline-754912</a><br><br> uint8_t Storage = 0;<br> using Amount = Bitfield<unsigned, 0, 4>;<br><br> Amount::fieldOf(Storage) = true;<br> unsigned Value = Amount::fieldOf(Storage);<br> bool Cond = Amount::isSet(Storage);<br><br><br>Proposal 1 has the inconvenience of polluting the llvm namespace but is also simpler implementation wise.<br>2 and 3 are more involved and add their own gotchas (cast operator, lifetime considerations) but feel more natural.<br></div><div><br></div><div>I would really appreciate guidance from the community on which syntax is preferred, or any other suggestions (`isSet` is not a great name)</div><div><br></div><div>Thank you!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 11, 2020 at 6:04 PM Guillaume Chatelet <<a href="mailto:gchatelet@google.com" target="_blank">gchatelet@google.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"><div>TL;DR: Have support in ADT for typed values packing into opaque scalar types</div><div> - Code & design choices: <a href="https://reviews.llvm.org/D81580" target="_blank">https://reviews.llvm.org/D81580</a><br></div><div> - Usage: <a href="https://reviews.llvm.org/differential/changeset/?ref=2005337&whitespace=ignore-most" target="_blank">https://reviews.llvm.org/differential/changeset/?ref=2005337&whitespace=ignore-most</a></div><div> - Example of rewrite: <a href="https://reviews.llvm.org/D81662" target="_blank">https://reviews.llvm.org/D81662</a></div><div><br></div><div>*CONTEXT*</div><div>There are places in LLVM where we need to pack typed fields into opaque values.<br></div><div><br></div><div>For instance, the `XXXInst` classes in `llvm/include/llvm/IR/Instructions.h` that extract information from `Value::SubclassData` via `getSubclassDataFromInstruction()`.</div><div>The bit twiddling is done manually: this impairs readability and prevent consistent handling of out of range values (e.g. <a href="https://github.com/llvm/llvm-project/blob/435b458ad0a4630e6126246a6865748104ccad06/llvm/include/llvm/IR/Instructions.h#L564" target="_blank">https://github.com/llvm/llvm-project/blob/435b458ad0a4630e6126246a6865748104ccad06/llvm/include/llvm/IR/Instructions.h#L564</a>).</div><div>More importantly, the bit pattern is scattered throughout the implementation making it hard to pack additional fields or check for overlapping bits.<br></div><div><br></div><div>I think it would be useful to have first class support for this use case in LLVM so here is a patch to get the discussion started <a href="https://reviews.llvm.org/D81580" target="_blank">https://reviews.llvm.org/D81580</a>. And here the `Instruction` code rewritten with it <a href="https://reviews.llvm.org/D81662" target="_blank">https://reviews.llvm.org/D81662</a>, I've added comments to highlight problems in the existing logic.</div><div><br></div><div>Feedback would be highly appreciated.</div><div></div></div>
</blockquote></div>
_______________________________________________<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>
</blockquote></div>