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>
Guillaume Chatelet via llvm-dev
2020-Jun-24 12:21 UTC
[llvm-dev] [RFC] Small Bitfield utilities
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>
Guillaume Chatelet via llvm-dev
2020-Jun-29 12:37 UTC
[llvm-dev] [RFC] Small Bitfield utilities
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>