[PATCH] D116469: [AMDGPU][KnownBits] Correct the known bits calculation for MUL_I24.

[Craig Topper via Phabricator via llvm-commits]

craig.topper created this revision.
craig.topper added reviewers: RKSimon, foad, arsenm, spatel, lebedev.ri.
Herald added subscribers: dexonsmith, kerbowa, hiraditya, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl.
craig.topper requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.

I'm not entirely sure, but based on how ComputeNumSignBits handles
ISD::MUL, I believe this code was miscounting the number of sign
bits.

As an example of an incorrect result let's say that countMinSignBits
returned 1 for the left hand side and 24 for the right hand side.
LHSValBits would be 23 and RHSValBits would be 0 and the sum would
be 23. This would cause the code to set 9 high bits as zero/one. Now
suppose the real values for the left side is 0x8000000 and the right
hand side is 0xffffff. The product is 0x00800000 which has 8 sign bits
not 9.

I've made KnownBits::countMinSignBits return 1 when no bits are known
since every value has 1 sign bit even if we don't know its value. This
should be fine for the other callers that max it with a value that is
at least 1.

The number of valid bits for the left and right operands is now
the number of non-sign bits + 1. If the sum of the valid bits of
the left and right sides exceeds 32, then the result may overflow and we
can't say anything about the sign of the result. If the sum is 32
or less then it won't overflow and we know the result has at least
1 sign bit.

For the previous example, the code will now calculate the left
side valid bits as 24 and the right side as 1. The sum will be 25
and the sign bits will be 32 - 25 + 1 which is 8, the correct value.

I don't know enough about AMDGPU to write a test for this. I just
noticed while looking at how KnownBits::countMinSignBits was used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116469

Files:
  llvm/include/llvm/Support/KnownBits.h
  llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp


Index: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
===================================================================
--- llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
+++ llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
@@ -4626,11 +4626,12 @@
     RHSKnown = RHSKnown.trunc(24);
 
     if (Opc == AMDGPUISD::MUL_I24) {
-      unsigned LHSValBits = 24 - LHSKnown.countMinSignBits();
-      unsigned RHSValBits = 24 - RHSKnown.countMinSignBits();
-      unsigned MaxValBits = std::min(LHSValBits + RHSValBits, 32u);
-      if (MaxValBits >= 32)
+      unsigned LHSValBits = 24 - LHSKnown.countMinSignBits() + 1;
+      unsigned RHSValBits = 24 - RHSKnown.countMinSignBits() + 1;
+      unsigned MaxValBits = LHSValBits + RHSValBits;
+      if (MaxValBits > 32)
         break;
+      unsigned SignBits = 32 - MaxValBits + 1;
       bool LHSNegative = LHSKnown.isNegative();
       bool LHSNonNegative = LHSKnown.isNonNegative();
       bool LHSPositive = LHSKnown.isStrictlyPositive();
@@ -4639,9 +4640,9 @@
       bool RHSPositive = RHSKnown.isStrictlyPositive();
 
       if ((LHSNonNegative && RHSNonNegative) || (LHSNegative && RHSNegative))
-        Known.Zero.setHighBits(32 - MaxValBits);
+        Known.Zero.setHighBits(SignBits);
       else if ((LHSNegative && RHSPositive) || (LHSPositive && RHSNegative))
-        Known.One.setHighBits(32 - MaxValBits);
+        Known.One.setHighBits(SignBits);
     } else {
       unsigned LHSValBits = 24 - LHSKnown.countMinLeadingZeros();
       unsigned RHSValBits = 24 - RHSKnown.countMinLeadingZeros();
Index: llvm/include/llvm/Support/KnownBits.h
===================================================================
--- llvm/include/llvm/Support/KnownBits.h
+++ llvm/include/llvm/Support/KnownBits.h
@@ -249,7 +249,8 @@
       return countMinLeadingZeros();
     if (isNegative())
       return countMinLeadingOnes();
-    return 0;
+    // Every value has at least 1 sign bit.
+    return 1;
   }
 
   /// Returns the maximum number of trailing zero bits possible.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116469.396843.patch
Type: text/x-patch
Size: 2019 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220101/f4227d8e/attachment.bin>