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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope updated this revision to Diff 221900.
bjope added a comment.

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.
===========================================


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68022/new/

https://reviews.llvm.org/D68022

Files:
  llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/test/Transforms/InstCombine/pr43376-getFlippedStrictnessPredicateAndConstant-assert.ll


Index: llvm/test/Transforms/InstCombine/pr43376-getFlippedStrictnessPredicateAndConstant-assert.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/InstCombine/pr43376-getFlippedStrictnessPredicateAndConstant-assert.ll
@@ -0,0 +1,36 @@
+; 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
+}
Index: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -5132,16 +5132,11 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D68022.221900.patch
Type: text/x-patch
Size: 2754 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190926/ca7aeb67/attachment.bin>