[llvm-dev] [RFC] Small Bitfield utilities

[Guillaume Chatelet via llvm-dev]

Documentation and code have been polished.
I'll go ahead and submit the version #3 of the design.
https://reviews.llvm.org/D82454

Thank you to everyone who contributed!

On Wed, Jun 24, 2020 at 2:21 PM Guillaume Chatelet <gchatelet at google.com>
wrote:

> 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/20200629/e8f0d109/attachment-0001.html>