[InstCombine] Don't assume CmpInst has been visited in getFlippedStrictnessPredicateAndConstant

[bjope]

Summary

Removing an assumption (assert) that the CmpInst already has been
simplified in getFlippedStrictnessPredicateAndConstant. Solution is
to simply bail out instead of hitting the assertion. Instead we
assume that any profitable rewrite will happen in the next iteration
of InstCombine.

The reason why we can't assume that the CmpInst already has been
simplified is that the worklist does not guarantee such an ordering.

Solves https://bugs.llvm.org/show_bug.cgi?id=43376

[lebedev.ri]

LG, sorry for the breakage!

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

5134–5135

if (auto* CI = dyn_cast<ConstantInt>(C)) {
  if (!ConstantIsOk(CI))
    return llvm::None;

5139–5140

Comment needs updating - the scalar case is no longer guaranteed to be handled,
so we can't talk about in the vector case.

[bjope]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

5139–5140

Maybe I've misunderstood something.

I've assumed that the ConstantIsOk check verified that the edge case had been handled (for the scalar case). And if not we bail out.
So in order to reach the code below this if-elseif-else, for scalars, the edge case should have been handled. Isn't that still true?

I can of course remove this TODO if you think that it isn't valid any longer.

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

5139–5140

// TODO? If the edge cases for vectors were guaranteed to be handled as they
// are for scalar

implies that we'll never get tautological inputs for scalars.
The old code asserted for that.
But now you remove that assert, thus saying there is no such guarantee scalars.
Therefore similarly we can't possibly ever have a similar guarantee for vectors.
At least that is how i parse all that after a quick look.

[bjope]

I don't think we need to talk about the "edge cases" at all any longer (after reading the comment from Roman once more, and analysing the code a little bit).

> This diff only updates the code comments.

[lebedev.ri]

Still LG.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

5139–5140

Please mark comments as done as appropriate

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

llvm::getFlippedStrictnessPredicateAndConstant(CmpInst::Predicate Pred,
   bool WillIncrement =
       UnsignedPred == ICmpInst::ICMP_ULE || UnsignedPred == ICmpInst::ICMP_UGT;
 
   // Check if the constant operand can be safely incremented/decremented
   // without overflowing/underflowing.
   auto ConstantIsOk = [WillIncrement, IsSigned](ConstantInt *C) {
     return WillIncrement ? !C->isMaxValue(IsSigned) : !C->isMinValue(IsSigned);
   };
 
-  // For scalars, SimplifyICmpInst should have already handled
-  // the edge cases for us, so we just assert on them.
-  // For vectors, we must handle the edge cases.
-  if (isa<ConstantInt>(C)) {
-    // A <= MAX -> TRUE ; A >= MIN -> TRUE
-    assert(ConstantIsOk(cast<ConstantInt>(C)));
+  if (auto *CI = dyn_cast<ConstantInt>(C)) {
+    // Bail out if the constant can't be safely incremented/decremented.
+    if (!ConstantIsOk(CI))
+      return llvm::None;
   } else if (Type->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. However, to do that,
-    // we would have to use insertelement/shufflevector to replace edge values.
     unsigned NumElts = Type->getVectorNumElements();
     for (unsigned i = 0; i != NumElts; ++i) {
       Constant *Elt = C->getAggregateElement(i);
       if (!Elt)
         return llvm::None;
 
       if (isa<UndefValue>(Elt))
         continue;
 

llvm/test/Transforms/InstCombine/pr43376-getFlippedStrictnessPredicateAndConstant-assert.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+; We used to hit an assertion in getFlippedStrictnessPredicateAndConstant due
+; to assuming that edge cases such as %cmp (ult x, 0) already has been
+; simplified. But that depends on the worklist order, so that is not always
+; guaranteed.
+
+define i16 @d(i16* %d.a, i16* %d.b) {
+; CHECK-LABEL: @d(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[T0:%.*]] = load i16, i16* [[D_A:%.*]], align 1
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp eq i16 [[T0]], 0
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[LAND_END:%.*]], label [[LAND_RHS:%.*]]
+; CHECK:       land.rhs:
+; CHECK-NEXT:    br label [[LAND_END]]
+; CHECK:       land.end:
+; CHECK-NEXT:    ret i16 -1
+;
+entry:
+  %t0 = load i16, i16* %d.a, align 1
+  %tobool = icmp ne i16 %t0, 0
+  br i1 %tobool, label %land.rhs, label %land.end
+
+land.rhs:
+  %t1 = load i16, i16* %d.b, align 1
+  %cmp = icmp ult i16 %t1, 0
+  br label %land.end
+
+land.end:
+  %t2 = phi i1 [ false, %entry ], [ %cmp, %land.rhs ]
+  %land.ext = zext i1 %t2 to i16
+  %mul = mul nsw i16 %land.ext, 3
+  %neg = xor i16 %mul, -1
+  ret i16 %neg
+}