[Support] Fix what I think is an off by 1 bug in UnsignedDivisionByConstantInfo.
authorCraig Topper <craig.topper@sifive.com>
Thu, 29 Dec 2022 17:35:34 +0000 (09:35 -0800)
committerCraig Topper <craig.topper@sifive.com>
Thu, 29 Dec 2022 17:42:50 +0000 (09:42 -0800)
The code in Hacker's Delight says
`nc = -1 - (-d)%d;`

But we have
`NC = AllOnes - (AllOnes-D)%D`

The Hacker's Delight code is written for the LeadingZeros==0 case.
`AllOnes - D` is not the same as `-d` from Hacker's Delight.

This patch changes the code to
`NC = AllOnes - (AllOnes+1-D)%D`

This will increment AllOnes to 0 in the LeadingZeros==0 case. This
will make it equivalent to -D. I believe this is also correct for
LeadingZeros>0.

At least for i8, i16, and i32 the only divisor that changes is
((1 << (BitWidth-1)) | 1). Or 127 for i8, 32769 for i16, and 2147483649
for i32. These are all large enough that the quotient is 0 or 1 so
InstCombine replaces them with an icmp and zext before SelectionDAG.

Reviewed By: lebedev.ri

Differential Revision: https://reviews.llvm.org/D140636

llvm/lib/Support/DivisionByConstantInfo.cpp

index c842240..b290760 100644 (file)
@@ -82,7 +82,9 @@ UnsignedDivisionByConstantInfo::get(const APInt &D, unsigned LeadingZeros) {
   APInt SignedMin = APInt::getSignedMinValue(D.getBitWidth());
   APInt SignedMax = APInt::getSignedMaxValue(D.getBitWidth());
 
-  APInt NC = AllOnes - (AllOnes - D).urem(D);
+  // Calculate NC, the largest dividend such that NC.urem(D) == D-1.
+  APInt NC = AllOnes - (AllOnes + 1 - D).urem(D);
+  assert(NC.urem(D) == D - 1 && "Unexpected NC value");
   unsigned P = D.getBitWidth() - 1; // initialize P
   APInt Q1, R1, Q2, R2;
   // initialize Q1 = 2P/NC; R1 = rem(2P,NC)