[llvm-dev] [RFC] Small Bitfield utilities

[Guillaume Chatelet via llvm-dev]

The code is now fairly stable and well tested but the syntax is not yet
final.
I would like to get some feedback from the community on alternative
syntaxes that have been suggested to me.

First, the designed API defines three functions:
 - get: unpacks the value from storage,
 - set: packs the provided value and stores it in storage,
 - test: efficiently returns whether the field is non zero.

Now, the three syntaxes:

1. Current API, purely functional
code: https://reviews.llvm.org/D81580

  uint8_t Storage = 0;
  using Amount = Bitfield<unsigned, 0, 4>;

  setField<Amount>(Storage, true);
  unsigned Value = getField<Amount>(Storage);
  bool Cond = testField<Amount>(Storage);

2. A more Object Oriented API
code: https://reviews.llvm.org/D82058

  uint8_t Storage = 0;
  using Amount = Bitfield<unsigned, 0, 4>;

  bitfield<Amount>(Storage) = true;
  unsigned Value = bitfield<Amount>(Storage);
  bool Cond = bitfield<Amount>(Storage).isSet();

3. In between
code: https://reviews.llvm.org/D82058#inline-754912

  uint8_t Storage = 0;
  using Amount = Bitfield<unsigned, 0, 4>;

  Amount::fieldOf(Storage) = true;
  unsigned Value = Amount::fieldOf(Storage);
  bool Cond = Amount::isSet(Storage);


Proposal 1 has the inconvenience of polluting the llvm namespace but is
also simpler implementation wise.
2 and 3 are more involved and add their own gotchas (cast operator,
lifetime considerations) but feel more natural.

I would really appreciate guidance from the community on which syntax is
preferred, or any other suggestions (`isSet` is not a great name)

Thank you!

On Thu, Jun 11, 2020 at 6:04 PM Guillaume Chatelet <gchatelet at google.com>
wrote:

> TL;DR: Have support in ADT for typed values packing into opaque scalar
> types
>  - Code & design choices: https://reviews.llvm.org/D81580
>  - Usage:
> https://reviews.llvm.org/differential/changeset/?ref=2005337&whitespace=ignore-most
>  - Example of rewrite: https://reviews.llvm.org/D81662
>
> *CONTEXT*
> There are places in LLVM where we need to pack typed fields into opaque
> values.
>
> For instance, the `XXXInst` classes in
> `llvm/include/llvm/IR/Instructions.h` that extract information from
> `Value::SubclassData` via `getSubclassDataFromInstruction()`.
> The bit twiddling is done manually: this impairs readability and prevent
> consistent handling of out of range values (e.g.
> https://github.com/llvm/llvm-project/blob/435b458ad0a4630e6126246a6865748104ccad06/llvm/include/llvm/IR/Instructions.h#L564
> ).
> More importantly, the bit pattern is scattered throughout the
> implementation making it hard to pack additional fields or check for
> overlapping bits.
>
> 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
> https://reviews.llvm.org/D81580. And here the `Instruction` code
> rewritten with it https://reviews.llvm.org/D81662, I've added comments to
> highlight problems in the existing logic.
>
> Feedback would be highly appreciated.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200623/0e7f3dbe/attachment.html>