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

[shchenz]

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.

[lkail]

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]

llvm/include/llvm/ADT/APInt.h

621

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

[shchenz]

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!

[lebedev.ri]

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?

[shchenz]

In D81325#2078182, @lebedev.ri wrote:

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]

llvm/include/llvm/ADT/APInt.h

621

Please update the setBitsWithWrap dox comment as well

[shchenz]

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

[shchenz]

llvm/include/llvm/ADT/APInt.h

621

Thanks. Updated accordingly.

[lebedev.ri]

LGTM

llvm/include/llvm/ADT/APInt.h

public:
   static APInt getBitsSet(unsigned numBits, unsigned loBit, unsigned hiBit) {
     assert(loBit <= hiBit && "loBit greater than hiBit");
     APInt Res(numBits, 0);
     Res.setBits(loBit, hiBit);
     return Res;
   }
 
   /// Wrap version of getBitsSet.
-  /// If \p hiBit is no less than \p loBit, this is same with getBitsSet.
-  /// If \p hiBit is less than \p loBit, the set bits "wrap". For example, with
-  /// parameters (32, 28, 4), you would get 0xF000000F.
+  /// If \p hiBit is bigger than \p loBit, this is same with getBitsSet.
+  /// If \p hiBit is not bigger than \p loBit, the set bits "wrap". For example,
+  /// with parameters (32, 28, 4), you would get 0xF000000F.
+  /// If \p hiBit is equal to \p loBit, you would get a result with all bits
+  /// set.
   static APInt getBitsSetWithWrap(unsigned numBits, unsigned loBit,
                                   unsigned hiBit) {
     APInt Res(numBits, 0);
     Res.setBitsWithWrap(loBit, hiBit);
     return Res;
   }
 
   /// Get a value with upper bits starting at loBit set.
#endif
   }
 
   /// Set the sign bit to 1.
   void setSignBit() {
     setBit(BitWidth - 1);
   }
 
   /// Set the bits from loBit (inclusive) to hiBit (exclusive) to 1.
-  /// This function handles "wrap" case when \p loBit > \p hiBit, and calls
-  /// setBits when \p loBit <= \p hiBit.
+  /// This function handles "wrap" case when \p loBit >= \p hiBit, and calls
+  /// setBits when \p loBit < \p hiBit.
   void setBitsWithWrap(unsigned loBit, unsigned hiBit) {
     assert(hiBit <= BitWidth && "hiBit out of range");
     assert(loBit <= BitWidth && "loBit out of range");
-    if (loBit <= hiBit) {
+    if (loBit < hiBit) {
       setBits(loBit, hiBit);
       return;
     }
     setLowBits(hiBit);
     setHighBits(BitWidth - loBit);
   }
 
   /// Set the bits from loBit (inclusive) to hiBit (exclusive) to 1.
   /// This function handles case when \p loBit <= \p hiBit.
   void setBits(unsigned loBit, unsigned hiBit) {
     assert(hiBit <= BitWidth && "hiBit out of range");

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp

for (MachineInstr &MI : MBB) {
 
         // Handle MBMI <= MEMI cases.
         APInt MaskMI = APInt::getBitsSetWithWrap(32, 32 - MEMI - 1, 32 - MBMI);
         // In MI, we only need low 32 bits of SrcMI, just consider about low 32
         // bit of SrcMI mask. Note that in APInt, lowerest bit is at index 0,
         // while in PowerPC ISA, lowerest bit is at index 63.
         APInt MaskSrc =
             APInt::getBitsSetWithWrap(32, 32 - MESrc - 1, 32 - MBSrc);
-        // Current APInt::getBitsSetWithWrap sets all bits to 0 if loBit is
-        // equal to highBit.
-        // If MBSrc - MESrc == 1, we expect a full set mask instead of Null.
-        if (SrcMaskFull && (MBSrc - MESrc == 1))
-          MaskSrc.setAllBits();
 
         APInt RotatedSrcMask = MaskSrc.rotl(SHMI);
         APInt FinalMask = RotatedSrcMask & MaskMI;
         uint32_t NewMB, NewME;
 
         // If final mask is 0, MI result should be 0 too.
         if (FinalMask.isNullValue()) {
           bool Is64Bit = (MI.getOpcode() == PPC::RLWINM8 ||

llvm/unittests/ADT/APIntTest.cpp

TEST(APIntTest, getBitsSetWithWrap) {
 
   APInt i127hi1lo1wrap = APInt::getBitsSetWithWrap(127, 126, 1);
   EXPECT_EQ(1u, i127hi1lo1wrap.countLeadingOnes());
   EXPECT_EQ(0u, i127hi1lo1wrap.countLeadingZeros());
   EXPECT_EQ(127u, i127hi1lo1wrap.getActiveBits());
   EXPECT_EQ(0u, i127hi1lo1wrap.countTrailingZeros());
   EXPECT_EQ(1u, i127hi1lo1wrap.countTrailingOnes());
   EXPECT_EQ(2u, i127hi1lo1wrap.countPopulation());
+
+  APInt i32hiequallowrap = APInt::getBitsSetWithWrap(32, 10, 10);
+  EXPECT_EQ(32u, i32hiequallowrap.countLeadingOnes());
+  EXPECT_EQ(0u, i32hiequallowrap.countLeadingZeros());
+  EXPECT_EQ(32u, i32hiequallowrap.getActiveBits());
+  EXPECT_EQ(0u, i32hiequallowrap.countTrailingZeros());
+  EXPECT_EQ(32u, i32hiequallowrap.countTrailingOnes());
+  EXPECT_EQ(32u, i32hiequallowrap.countPopulation());
 }
 
 TEST(APIntTest, getHighBitsSet) {
   APInt i64hi32 = APInt::getHighBitsSet(64, 32);
   EXPECT_EQ(32u, i64hi32.countLeadingOnes());
   EXPECT_EQ(0u, i64hi32.countLeadingZeros());
   EXPECT_EQ(64u, i64hi32.getActiveBits());
   EXPECT_EQ(32u, i64hi32.countTrailingZeros());