[llvm-dev] [RFC] Small Bitfield utilities

[Guillaume Chatelet via llvm-dev]

Hi River,

I've to say I really like your suggestion.
I went ahead and implemented it as https://reviews.llvm.org/D82454

I'll wait a bit more for other people to chime in but overall it has my
preference.

On Tue, Jun 23, 2020 at 9:51 PM River Riddle <riddleriver at gmail.com> wrote:

> 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/20200624/2aff892b/attachment.html>