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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as done.
shchenz 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:
> > > 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`.


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

https://reviews.llvm.org/D69032