[llvm] 2872987 - [InstCombine] Fix InstCombinerImpl::foldICmpMulConstant for nsw and nuw mul with unsigned compare.

[Craig Topper via llvm-commits]

Author: Craig Topper
Date: 2023-02-14T23:43:17-08:00
New Revision: 2872987e5e91cbda423a3b2be061756a116902b6

URL: https://github.com/llvm/llvm-project/commit/2872987e5e91cbda423a3b2be061756a116902b6
DIFF: https://github.com/llvm/llvm-project/commit/2872987e5e91cbda423a3b2be061756a116902b6.diff

LOG: [InstCombine] Fix InstCombinerImpl::foldICmpMulConstant for nsw and nuw mul with unsigned compare.

If we have both an nsw and nuw flag, we would see the nsw flag
first and only handle signed comparisons.

This patch ignores the nsw flag if the comparison isn't signed.

Reviewed By: nikic

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

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
    llvm/test/Transforms/InstCombine/icmp-mul.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index c8010b7fbf3a..20622da742b2 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -2061,35 +2061,37 @@ Instruction *InstCombinerImpl::foldICmpMulConstant(ICmpInst &Cmp,
     }
   }
 
-  if (!Mul->hasNoSignedWrap() && !Mul->hasNoUnsignedWrap())
-    return nullptr;
-
   // With a matching no-overflow guarantee, fold the constants:
   // (X * MulC) < C --> X < (C / MulC)
   // (X * MulC) > C --> X > (C / MulC)
   // TODO: Assert that Pred is not equal to SGE, SLE, UGE, ULE?
   Constant *NewC = nullptr;
-  if (Mul->hasNoSignedWrap()) {
+  if (Mul->hasNoSignedWrap() && ICmpInst::isSigned(Pred)) {
     // MININT / -1 --> overflow.
     if (C.isMinSignedValue() && MulC->isAllOnes())
       return nullptr;
     if (MulC->isNegative())
       Pred = ICmpInst::getSwappedPredicate(Pred);
 
-    if (Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGE)
+    if (Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGE) {
       NewC = ConstantInt::get(
           MulTy, APIntOps::RoundingSDiv(C, *MulC, APInt::Rounding::UP));
-    if (Pred == ICmpInst::ICMP_SLE || Pred == ICmpInst::ICMP_SGT)
+    } else {
+      assert((Pred == ICmpInst::ICMP_SLE || Pred == ICmpInst::ICMP_SGT) &&
+             "Unexpected predicate");
       NewC = ConstantInt::get(
           MulTy, APIntOps::RoundingSDiv(C, *MulC, APInt::Rounding::DOWN));
-  } else {
-    assert(Mul->hasNoUnsignedWrap() && "Expected mul nuw");
-    if (Pred == ICmpInst::ICMP_ULT || Pred == ICmpInst::ICMP_UGE)
+    }
+  } else if (Mul->hasNoUnsignedWrap() && ICmpInst::isUnsigned(Pred)) {
+    if (Pred == ICmpInst::ICMP_ULT || Pred == ICmpInst::ICMP_UGE) {
       NewC = ConstantInt::get(
           MulTy, APIntOps::RoundingUDiv(C, *MulC, APInt::Rounding::UP));
-    if (Pred == ICmpInst::ICMP_ULE || Pred == ICmpInst::ICMP_UGT)
+    } else {
+      assert((Pred == ICmpInst::ICMP_ULE || Pred == ICmpInst::ICMP_UGT) &&
+             "Unexpected predicate");
       NewC = ConstantInt::get(
           MulTy, APIntOps::RoundingUDiv(C, *MulC, APInt::Rounding::DOWN));
+    }
   }
 
   return NewC ? new ICmpInst(Pred, X, NewC) : nullptr;

diff  --git a/llvm/test/Transforms/InstCombine/icmp-mul.ll b/llvm/test/Transforms/InstCombine/icmp-mul.ll
index 13e21b59676e..4268bbba416f 100644
--- a/llvm/test/Transforms/InstCombine/icmp-mul.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-mul.ll
@@ -134,11 +134,10 @@ define i1 @ult_rem_zero(i8 %x) {
 }
 
 ; Same as above, but with nsw flag too.
-; FIXME: This should be optimized like above.
+; This used to not optimize due to nsw being prioritized too much.
 define i1 @ult_rem_zero_nsw(i8 %x) {
 ; CHECK-LABEL: @ult_rem_zero_nsw(
-; CHECK-NEXT:    [[A:%.*]] = mul nuw nsw i8 [[X:%.*]], 7
-; CHECK-NEXT:    [[B:%.*]] = icmp ult i8 [[A]], 21
+; CHECK-NEXT:    [[B:%.*]] = icmp ult i8 [[X:%.*]], 3
 ; CHECK-NEXT:    ret i1 [[B]]
 ;
   %a = mul nuw nsw i8 %x, 7
@@ -157,11 +156,10 @@ define i1 @ult_rem_nz(i8 %x) {
 }
 
 ; Same as above, but with nsw flag too.
-; FIXME: This should be optimized like above.
+; This used to not optimize due to nsw being prioritized too much.
 define i1 @ult_rem_nz_nsw(i8 %x) {
 ; CHECK-LABEL: @ult_rem_nz_nsw(
-; CHECK-NEXT:    [[A:%.*]] = mul nuw nsw i8 [[X:%.*]], 5
-; CHECK-NEXT:    [[B:%.*]] = icmp ult i8 [[A]], 21
+; CHECK-NEXT:    [[B:%.*]] = icmp ult i8 [[X:%.*]], 5
 ; CHECK-NEXT:    ret i1 [[B]]
 ;
   %a = mul nuw nsw i8 %x, 5
@@ -212,11 +210,10 @@ define i1 @ugt_rem_zero(i8 %x) {
 }
 
 ; Same as above, but with nsw flag too.
-; FIXME: This should be optimized like above.
+; This used to not optimize due to nsw being prioritized too much.
 define i1 @ugt_rem_zero_nsw(i8 %x) {
 ; CHECK-LABEL: @ugt_rem_zero_nsw(
-; CHECK-NEXT:    [[A:%.*]] = mul nuw nsw i8 [[X:%.*]], 7
-; CHECK-NEXT:    [[B:%.*]] = icmp ugt i8 [[A]], 21
+; CHECK-NEXT:    [[B:%.*]] = icmp ugt i8 [[X:%.*]], 3
 ; CHECK-NEXT:    ret i1 [[B]]
 ;
   %a = mul nuw nsw i8 %x, 7
@@ -235,11 +232,10 @@ define i1 @ugt_rem_nz(i8 %x) {
 }
 
 ; Same as above, but with nsw flag too.
-; FIXME: This should be optimized like above.
+; This used to not optimize due to nsw being prioritized too much.
 define i1 @ugt_rem_nz_nsw(i8 %x) {
 ; CHECK-LABEL: @ugt_rem_nz_nsw(
-; CHECK-NEXT:    [[A:%.*]] = mul nuw nsw i8 [[X:%.*]], 5
-; CHECK-NEXT:    [[B:%.*]] = icmp ugt i8 [[A]], 21
+; CHECK-NEXT:    [[B:%.*]] = icmp ugt i8 [[X:%.*]], 4
 ; CHECK-NEXT:    ret i1 [[B]]
 ;
   %a = mul nuw nsw i8 %x, 5
@@ -998,9 +994,7 @@ define i1 @splat_mul_known_lz(i32 %x) {
 
 define i1 @splat_mul_unknown_lz(i32 %x) {
 ; CHECK-LABEL: @splat_mul_unknown_lz(
-; CHECK-NEXT:    [[Z:%.*]] = zext i32 [[X:%.*]] to i128
-; CHECK-NEXT:    [[M:%.*]] = mul nuw nsw i128 [[Z]], 18446744078004518913
-; CHECK-NEXT:    [[R:%.*]] = icmp ult i128 [[M]], 39614081257132168796771975168
+; CHECK-NEXT:    [[R:%.*]] = icmp sgt i32 [[X:%.*]], -1
 ; CHECK-NEXT:    ret i1 [[R]]
 ;
   %z = zext i32 %x to i128