This is an archive of the discontinued LLVM Phabricator instance.

[APInt] set all bits for getBitsSetWithWrap if loBit == hiBit
ClosedPublic

Authored by shchenz on Jun 5 2020, 7:37 PM.

Details

Summary

For now,
getBitsSetWithWrap(unsigned numBits, unsigned loBit, unsigned hiBit) & getBitsSet(unsigned numBits, unsigned loBit, unsigned hiBit) both do nothing if loBit == hiBit.

We need to differentiate them.
if loBit == hiBit, getBitsSetWithWrap sets all bits. and getBitsSet does nothing.

Diff Detail

Event Timeline

shchenz created this revision.Jun 5 2020, 7:37 PM
lkail added a subscriber: lkail.Jun 6 2020, 2:49 AM
lkail added inline comments.
llvm/include/llvm/ADT/APInt.h
1461

Have no knowledge of this method's semantics, what this branch does should equal to the block following.

RKSimon added inline comments.Jun 6 2020, 5:53 AM
llvm/include/llvm/ADT/APInt.h
621

We probably should explicitly explain the loBit == hiBit case here and for setBitsWithWrap

shchenz updated this revision to Diff 269018.Jun 6 2020, 6:11 AM
shchenz marked 2 inline comments as done.

address code review comments:
1: simplify the code logic
2: add explicit comment for the case loBit == hiBit

llvm/include/llvm/ADT/APInt.h
621

Good idea.

1461

Good catch!

I assume all PPC usages of this function is happy with the change?
It is also used in GlobalISel's LegalizerHelper::lowerInsert(), is it also okay with this?

I assume all PPC usages of this function is happy with the change?
It is also used in GlobalISel's LegalizerHelper::lowerInsert(), is it also okay with this?

APInt MaskVal = APInt::getBitsSetWithWrap(
    DstTy.getSizeInBits(), Offset + InsertTy.getSizeInBits(), Offset);

I am thinking the change will not impact the above use as InsertTy.getSizeInBits() should not be 0?

RKSimon added inline comments.Jun 7 2020, 8:51 AM
llvm/include/llvm/ADT/APInt.h
621

Please update the setBitsWithWrap dox comment as well

shchenz updated this revision to Diff 269063.Jun 7 2020, 9:09 AM

address comments:
1: add explicit comment for the case loBit == hiBit in function setBitsWithWrap as well

shchenz marked an inline comment as done.Jun 7 2020, 9:10 AM
shchenz added inline comments.
llvm/include/llvm/ADT/APInt.h
621

Thanks. Updated accordingly.

This revision is now accepted and ready to land.Jun 8 2020, 10:13 AM
This revision was automatically updated to reflect the committed changes.