Page MenuHomePhabricator

[APInt] add wrapping support for APInt::setBits
ClosedPublic

Authored by shchenz on Oct 16 2019, 5:44 AM.

Details

Summary

This was supportted before https://reviews.llvm.org/rL301769.

In rL301769, It removed this support for no uses for this feature.
But getBitsSet which calls setBits still comment If hiBit is less than loBit then the set bits "wrap". . This is a little confusing.

I add this feature back because in PowerPC, I need it to combine two PPC::RLWINM which will be posted later. In PPC::RLWINM wrapping set bits is supportted.

Diff Detail

Event Timeline

shchenz created this revision.Oct 16 2019, 5:44 AM

I'd prefer to see the actual patch that makes use of this.
This doesn't address the cost issue noted in rL301769.

lebedev.ri requested changes to this revision.Oct 16 2019, 11:46 AM

(just marking as reviewed)

This revision now requires changes to proceed.Oct 16 2019, 11:46 AM
xbolva00 resigned from this revision.Nov 12 2019, 3:25 AM
shchenz updated this revision to Diff 229755.Nov 18 2019, 12:12 AM

Use new function setBitsWithWrap to avoid setBits can not be inlined to setLowBits, setHighBits, and setBitsFrom

Sorry for the delay. I use the method suggested in https://reviews.llvm.org/rL301769. The patch using this feature is https://reviews.llvm.org/D70374.
We want to set bits for mask of PPC instruciton rlwinm. That instruction supports wrap masks.

shchenz updated this revision to Diff 229756.Nov 18 2019, 12:18 AM

fix one formatting issue.

lebedev.ri added inline comments.Nov 18 2019, 12:22 AM
llvm/include/llvm/ADT/APInt.h
606–611

These changes are still there

lebedev.ri added inline comments.Nov 18 2019, 12:47 AM
llvm/include/llvm/ADT/APInt.h
606–611

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;
}
shchenz marked an inline comment as done.Nov 18 2019, 1:00 AM
shchenz added inline comments.
llvm/include/llvm/ADT/APInt.h
606–611

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?

lebedev.ri added inline comments.Nov 18 2019, 1:09 AM
llvm/include/llvm/ADT/APInt.h
606–611

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().

shchenz marked an inline comment as done.Nov 18 2019, 1:31 AM
shchenz added inline comments.
llvm/include/llvm/ADT/APInt.h
606–611

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.

I think setBitsWithWrap should call setBits if there is no wrap and call setLowBits and setHighBits if there is wrap. Matching the behavior setBits used to have. Any incorrect comments on setBits or getBitsSet should be fixed.

This gives places that want wrap the support they need inside of a APInt but doesn’t slow down or increase the code size of any of the existing callers of setBits.

lebedev.ri added inline comments.Nov 18 2019, 2:15 AM
llvm/include/llvm/ADT/APInt.h
606–611

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.

lebedev.ri added inline comments.Nov 18 2019, 5:07 AM
llvm/include/llvm/ADT/APInt.h
606–611

... 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;
}
1451–1456

Let's just go other way around, and early-return if no-wrap?

shchenz updated this revision to Diff 229954.Nov 18 2019, 5:56 PM
shchenz marked an inline comment as done.

Use two interface getBitsSet and getBitsSetWithWrap.

To be honest, I don't like this fix, it is a patch-like fix. In order to protect legacy usage, we create two functionally overlapping interfaces for both getBitsSet and setBits. But I can not find another better way to not change legacy usage for these functions, so let it be like now.

lebedev.ri accepted this revision.Nov 18 2019, 11:24 PM

LGTM, thank you.

This revision is now accepted and ready to land.Nov 18 2019, 11:24 PM
craig.topper accepted this revision.Nov 18 2019, 11:43 PM

LGTM too

This revision was automatically updated to reflect the committed changes.