llvm dev - Jun 2020 - [RFC] Small Bitfield utilities

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/20200611/e314b55a/attachment.html>

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>

Hi Guillaume,

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.

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:

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

  Bitfield::set<Amount>(Storage, true);
  unsigned Value = Bitfield::get<Amount>(Storage);
  bool Cond = Bitfield::test<Amount>(Storage);
```

^ Seems good to me because it's clear what the API is doing, even without
knowing what `Amount` is. WDYT?

-- River


On Tue, Jun 23, 2020 at 9:40 AM Guillaume Chatelet via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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.
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200623/b6c52a42/attachment.html>