Guillaume Chatelet via llvm-dev
2020-Jun-11 16:04 UTC
[llvm-dev] [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>
Guillaume Chatelet via llvm-dev
2020-Jun-23 16:38 UTC
[llvm-dev] [RFC] Small Bitfield utilities
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>