[llvm-dev] [RFC] Small Bitfield utilities

[River Riddle via llvm-dev]

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>