[InstCombine] check vector elements before trying to transform LE/GE vector icmp (PR27756)

[spatel]

Summary

Fix a bug introduced with rL269426 :
[InstCombine] canonicalize* LE/GE vector integer comparisons to LT/GT (PR26701, PR26819)

We were assuming that a ConstantDataVector / ConstantVector / ConstantAggregateZero operand of an ICMP was composed of ConstantInt elements, but it might have ConstantExpr or UndefValue elements. Handle those appropriately.

[majnemer]

lib/Transforms/InstCombine/InstCombineCompares.cpp

3134–3135 ↗

(On Diff #57363)

Can we just make this Op1->getType()->isVectorTy() ?

[spatel]

lib/Transforms/InstCombine/InstCombineCompares.cpp

3134–3135 ↗

(On Diff #57363)

We still need to make sure Op1 is a Constant, but yes, I think that will be cleaner.

[spatel]

Patch updated:
Simplify check for transform to use operand type (we just need a vector constant of some kind).

[chandlerc]

lib/Transforms/InstCombine/InstCombineCompares.cpp

3138–3140 ↗

(On Diff #57372)

Instead of the bool flags here, I would pass in the predicate value and handle that internally.

With that, I tihnk you can completely eliminate the switch in the calling code as the only other difference is 1 vs. -1.

[majnemer]

lib/Transforms/InstCombine/InstCombineCompares.cpp

3165–3185 ↗

(On Diff #57372)

I'd recommend passing I.isSigned() in instead of hard-coding true/false. Makes it harder to make a mistake when updating this switch/case.

[spatel]

lib/Transforms/InstCombine/InstCombineCompares.cpp

3138–3140 ↗

(On Diff #57372)

We still have to choose 1 of the 4 predicate values, but I'll take the anti-switch bait. :)
Lets try for max density via ?: and see if that reads better.

[spatel]

Patch updated:

1. Refactored to use the same main logic path for scalar/vector.
2. Replace switches with ?: ops.
3. Eliminate lambda.
4. Remove unused Builder param.

[chandlerc]

LGTM. I think this could be cleaned up some more, but I think its fine for you to land as-is -- it'll be easier to just clean up the formatting after-the-fact.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp

if (isChainSelectCmpBranch(SI) && Icmp->getPredicate() == ICmpInst::ICMP_EQ) {
     }
   }
   return false;
 }
 
 /// If we have an icmp le or icmp ge instruction with a constant operand, turn
 /// it into the appropriate icmp lt or icmp gt instruction. This transform
 /// allows them to be folded in visitICmpInst.
-static ICmpInst *canonicalizeCmpWithConstant(ICmpInst &I,
-                                             InstCombiner::BuilderTy &Builder) {
+static ICmpInst *canonicalizeCmpWithConstant(ICmpInst &I) {
+  ICmpInst::Predicate Pred = I.getPredicate();
+  if (Pred != ICmpInst::ICMP_SLE && Pred != ICmpInst::ICMP_SGE &&
+      Pred != ICmpInst::ICMP_ULE && Pred != ICmpInst::ICMP_UGE)
+    return nullptr;
+
   Value *Op0 = I.getOperand(0);
   Value *Op1 = I.getOperand(1);
-
-  if (auto *Op1C = dyn_cast<ConstantInt>(Op1)) {
-    // For scalars, SimplifyICmpInst has already handled the edge cases for us,
-    // so we just assert on them.
-    APInt Op1Val = Op1C->getValue();
-    switch (I.getPredicate()) {
-    case ICmpInst::ICMP_ULE:
-      assert(!Op1C->isMaxValue(false)); // A <=u MAX -> TRUE
-      return new ICmpInst(ICmpInst::ICMP_ULT, Op0, Builder.getInt(Op1Val + 1));
-    case ICmpInst::ICMP_SLE:
-      assert(!Op1C->isMaxValue(true));  // A <=s MAX -> TRUE
-      return new ICmpInst(ICmpInst::ICMP_SLT, Op0, Builder.getInt(Op1Val + 1));
-    case ICmpInst::ICMP_UGE:
-      assert(!Op1C->isMinValue(false)); // A >=u MIN -> TRUE
-      return new ICmpInst(ICmpInst::ICMP_UGT, Op0, Builder.getInt(Op1Val - 1));
-    case ICmpInst::ICMP_SGE:
-      assert(!Op1C->isMinValue(true));  // A >=s MIN -> TRUE
-      return new ICmpInst(ICmpInst::ICMP_SGT, Op0, Builder.getInt(Op1Val - 1));
-    default:
-      return nullptr;
-    }
-  }
+  auto *Op1C = dyn_cast<Constant>(Op1);
+  if (!Op1C)
 
-  // The usual vector types are ConstantDataVector. Exotic vector types are
-  // ConstantVector. Zeros are special. They all derive from Constant.
-  if (isa<ConstantDataVector>(Op1) || isa<ConstantVector>(Op1) ||
-      isa<ConstantAggregateZero>(Op1)) {
-    Constant *Op1C = cast<Constant>(Op1);
-    Type *Op1Type = Op1->getType();
-    unsigned NumElts = Op1Type->getVectorNumElements();
-
-    // Set the new comparison predicate and splat a vector of 1 or -1 to
-    // increment or decrement the vector constants. But first, check that no
-    // elements of the constant vector would overflow/underflow when we
-    // increment/decrement the constants.
-    //
+  // Check if the constant operand can be safely incremented/decremented without
+  // overflowing/underflowing. For scalars, SimplifyICmpInst has already handled
+  // the edge cases for us, so we just assert on them. For vectors, we must
+  // handle the edge cases.
+  bool IsSigned = I.isSigned();
+  bool IsLE = (Pred == ICmpInst::ICMP_SLE || Pred == ICmpInst::ICMP_ULE);
+  if (auto *CI = dyn_cast<ConstantInt>(Op1C)) {
+    // A <= MAX -> TRUE ; A >= MIN -> TRUE
+    assert(IsLE ? !CI->isMaxValue(IsSigned) : !CI->isMinValue(IsSigned));
+  } else if (Op1Type->isVectorTy()) {
     // TODO? If the edge cases for vectors were guaranteed to be handled as they
-    // are for scalar, we could remove the min/max checks here. However, to do
-    // that, we would have to use insertelement/shufflevector to replace edge
-    // values.
-    
-    CmpInst::Predicate NewPred;
-    Constant *OneOrNegOne = nullptr;
-    switch (I.getPredicate()) {
-    case ICmpInst::ICMP_ULE:
-      for (unsigned i = 0; i != NumElts; ++i)
-        if (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMaxValue(false))
-          return nullptr;
-      NewPred = ICmpInst::ICMP_ULT;
-      OneOrNegOne = ConstantInt::get(Op1Type, 1);
-      break;
-    case ICmpInst::ICMP_SLE:
-      for (unsigned i = 0; i != NumElts; ++i)
-        if (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMaxValue(true))
-          return nullptr;
-      NewPred = ICmpInst::ICMP_SLT;
-      OneOrNegOne = ConstantInt::get(Op1Type, 1);
-      break;
-    case ICmpInst::ICMP_UGE:
-      for (unsigned i = 0; i != NumElts; ++i)
-        if (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMinValue(false))
-          return nullptr;
-      NewPred = ICmpInst::ICMP_UGT;
-      OneOrNegOne = ConstantInt::get(Op1Type, -1);
-      break;
-    case ICmpInst::ICMP_SGE:
-      for (unsigned i = 0; i != NumElts; ++i)
-        if (cast<ConstantInt>(Op1C->getAggregateElement(i))->isMinValue(true))
-          return nullptr;
-      NewPred = ICmpInst::ICMP_SGT;
-      OneOrNegOne = ConstantInt::get(Op1Type, -1);
-      break;
-    default:
-      return nullptr;
-    }
+    // are for scalar, we could remove the min/max checks. However, to do that,
+    // we would have to use insertelement/shufflevector to replace edge values.
+    unsigned NumElts = Op1Type->getVectorNumElements();
+    for (unsigned i = 0; i != NumElts; ++i) {
+      Constant *Elt = Op1C->getAggregateElement(i);
+      if (isa<UndefValue>(Elt))
+        continue;
+      // Bail out if we can't determine if this constant is min/max or if we
+      // know that this constant is min/max.
+      auto *CI = dyn_cast<ConstantInt>(Elt);
+      if (!CI || (IsLE ? CI->isMaxValue(IsSigned) : CI->isMinValue(IsSigned)))
+    }
+  } else {
+    // ConstantExpr?
 
-    return new ICmpInst(NewPred, Op0, ConstantExpr::getAdd(Op1C, OneOrNegOne));
-  }
+  // Increment or decrement the constant and set the new comparison predicate:
+  // ULE -> ULT ; UGE -> UGT ; SLE -> SLT ; SGE -> SGT
+  Constant *OneOrNegOne = ConstantInt::get(Op1Type, IsLE ? 1 : -1);
+  CmpInst::Predicate NewPred = IsLE ? ICmpInst::ICMP_ULT: ICmpInst::ICMP_UGT;
+  NewPred = IsSigned ? ICmpInst::getSignedPredicate(NewPred) : NewPred;
 
-  return nullptr;
-}
-
 Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
   bool Changed = false;
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
   unsigned Op0Cplxity = getComplexity(Op0);
   unsigned Op1Cplxity = getComplexity(Op1);
 
   /// Orders the operands of the compare so that they are listed from most
   /// complex to least complex.  This puts constants before unary operators,
case ICmpInst::ICMP_SGE:
       // FALL THROUGH
     case ICmpInst::ICMP_SLE: {               //  icmp sle i1 A, B -> A | ~B
       Value *Not = Builder->CreateNot(Op1, I.getName()+"tmp");
       return BinaryOperator::CreateOr(Not, Op0);
     }
     }
   }
 
-  if (ICmpInst *NewICmp = canonicalizeCmpWithConstant(I, *Builder))
+  if (ICmpInst *NewICmp = canonicalizeCmpWithConstant(I))
     return NewICmp;
 
   unsigned BitWidth = 0;
   if (Ty->isIntOrIntVectorTy())
     BitWidth = Ty->getScalarSizeInBits();
   else // Get pointer size.
     BitWidth = DL.getTypeSizeInBits(Ty->getScalarType());
 

llvm/trunk/test/Transforms/InstCombine/icmp-vec.ll

 ; CHECK-LABEL: @ule_max(
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ule <2 x i3> %x, <i3 -1, i3 1>
 ; CHECK-NEXT:    ret <2 x i1> [[CMP]]
 ;
   %cmp = icmp ule <2 x i3> %x, <i3 7, i3 1>
   ret <2 x i1> %cmp
 }
 
+; If we can't determine if a constant element is min/max (eg, it's a ConstantExpr), do nothing.
+
+define <2 x i1> @PR27756_1(<2 x i8> %a) {
+; CHECK-LABEL: @PR27756_1(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sle <2 x i8> %a, <i8 bitcast (<2 x i4> <i4 1, i4 2> to i8), i8 0>
+; CHECK-NEXT:    ret <2 x i1> [[CMP]]
+;
+  %cmp = icmp sle <2 x i8> %a, <i8 bitcast (<2 x i4> <i4 1, i4 2> to i8), i8 0>
+  ret <2 x i1> %cmp
+}
+
+; Undef elements don't prevent the transform of the comparison.
+
+define <2 x i1> @PR27756_2(<2 x i8> %a) {
+; CHECK-LABEL: @PR27756_2(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt <2 x i8> %a, <i8 undef, i8 1>
+; CHECK-NEXT:    ret <2 x i1> [[CMP]]
+;
+  %cmp = icmp sle <2 x i8> %a, <i8 undef, i8 0>
+  ret <2 x i1> %cmp
+}
+