<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">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">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">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>