[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;
----------------
lebedev.ri wrote:
> 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.
... and now you can //actually// do
```
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;
}
``` 


================
Comment at: llvm/include/llvm/ADT/APInt.h:1439-1444
+    if (loBit > hiBit) {
+      setLowBits(hiBit);
+      setHighBits(BitWidth - loBit);
+      return;
+    }
+    setBits(loBit, hiBit);
----------------
Let's just go other way around, and early-return if no-wrap?


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

https://reviews.llvm.org/D69032