[PATCH] D69032: [APInt] add wrapping support for APInt::setBits

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/ADT/APInt.h:606-613
   static APInt getBitsSet(unsigned numBits, unsigned loBit, unsigned hiBit) {
     APInt Res(numBits, 0);
-    Res.setBits(loBit, hiBit);
+    if (loBit <= hiBit)
+      Res.setBits(loBit, hiBit);
+    else
+      Res.setBitsWithWrap(loBit, hiBit);
     return Res;
----------------
shchenz wrote:
> lebedev.ri wrote:
> > shchenz wrote:
> > > lebedev.ri wrote:
> > > > lebedev.ri wrote:
> > > > > These changes are still there
> > > > To reword, the suggestion is to
> > > > 
> > > > ```
> > > >   static APInt getBitsSet(unsigned numBits, unsigned loBit, unsigned hiBit) {
> > > >     APInt Res(numBits, 0);
> > > >     Res.setBits(loBit, hiBit);
> > > >     return Res;
> > > >   }
> > > > 
> > > >   static APInt getBitsSetWithWrap(unsigned numBits, unsigned loBit, unsigned hiBit) {
> > > >     APInt Res(numBits, 0);
> > > >     Res.setBitsWithWrap(loBit, hiBit);
> > > >     return Res;
> > > >   }
> > > > ```
> > > I think if we want to avoid recursion in `setBits`, we just need to split `setBits` for wrap and non-wrap.   I am not sure the benifit of spliting `getBitsSet`, could you point it out to me?
> > Presumably all existing users of `getBitsSet()` satisfy `loBit <= hiBit` precondition,
> > and don't need the new `setBitsWithWrap()` behaviour. Which means the `getBitsSet()`
> > change as-is will likely affect the performance of every current `getBitsSet()` user.
> > It may be best to simply not touch existing `getBitsSet()`, but add `getBitsSetWithWrap()`.
> Now, after the change, every user of `getBitsSet` will check `if (loBit <= hiBit)` and all users will not go into new added `setBitsWithWrap`.
> 
> The perf impact mentioned in rL301769 is `setBits` can not call `setLowBits` or `setHighBits`. If  we do, `setBits` will become a recurive function, and `setBits` can not be inlined to `setLowBits` or `setHighBits`.  So we add another `setBitsWithWrap` function to avoid such case. For `getBitsSet`, there is no such issue?
> 
> And if we use `getBitsSet` and `getBitsSetWithWrap`, we have to call getBitsSet outside of `APInt`, like:
> ```
> if (loBit > hiBit)
>   getBitsSetWithWrap()
> else
>   getBitsSet()
> ```
> This does not match the comment for `getBitsSet`.
Ah wait, i missed that we still need the check that there is a wrap, right.
Then i'd +1 to what @craig.topper said, and my original suggestion `getBitsSetWithWrap()` is applicable again.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69032/new/

https://reviews.llvm.org/D69032