[JumpThreading] Change asserts for WantInteger into actual checks

[paquette]

Summary

After e734e8286b4b521d829aaddb6d1cbbd264953625, it is possible to end up in a situation where an indirectbr is fed by a cast, which is in turn fed by an operation which only produces integers.

indirectbr expects a block address, however these operations can't produce that.

There were several asserts in computeValueKnownInPredecessorsImpl which check that we're not looking for a block address if we're walking through something which can never produce one.

Since it's now possible to hit these asserts, this changes them into actual checks which return false if Preference is not WantInteger.

This adds a testcase which verifies that we don't crash anymore in these situations.

[nikic]

From someone who isn't very familiar with this code: Why do we need the preference flag in the first place? As indirectbr isn't particularly common, can't we check whether the result is a BlockAddress after the fact?

[paquette]

In D99814#2668722, @nikic wrote:

From someone who isn't very familiar with this code: Why do we need the preference flag in the first place? As indirectbr isn't particularly common, can't we check whether the result is a BlockAddress after the fact?

I'm not hugely familiar with this code either, so I can't really say why it exists. It seems like it's mostly only used for getKnownConstant.

I think it might be possible to refactor. It's nice that we can stop the recursive search in computeValueKnownInPredecessorsImpl early by using it though. I'm a little surprised that it wasn't used in that way before.

I don't really have strong opinions either way, WDYT?

[aqjune]

This tracks down to a commit in 2010: https://github.com/llvm/llvm-project/commit/d9df6eaa9ce20
Interestingly the motivation of introducing ConstantPreference was to properly deal with indirectbr. :)
I have no strong preference either.

[paquette]

Personally, I'd like to avoid refactoring in this patch just to keep things small and avoid any unintended side-effects. Does that sound okay? I think refactoring is probably a good idea in a follow-up. :)

[aemerson]

llvm/test/Transforms/JumpThreading/indirectbr-cast-int-op.ll

19

Don't use undef here?

[aemerson]

LGTM with the test nit unless anyone else has objections.

llvm/lib/Transforms/Scalar/JumpThreading.cpp

if (FreezeInst *FI = dyn_cast<FreezeInst>(I)) {
     });
 
     return !Result.empty();
   }
 
   // Handle some boolean conditions.
   if (I->getType()->getPrimitiveSizeInBits() == 1) {
     using namespace PatternMatch;
-
-    assert(Preference == WantInteger && "One-bit non-integer type?");
+    if (Preference != WantInteger)
+      return false;
     // X | true -> true
     // X & false -> false
     Value *Op0, *Op1;
     if (match(I, m_LogicalOr(m_Value(Op0), m_Value(Op1))) ||
         match(I, m_LogicalAnd(m_Value(Op0), m_Value(Op1)))) {
       PredValueInfoTy LHSVals, RHSVals;
 
       computeValueKnownInPredecessorsImpl(Op0, BB, LHSVals, WantInteger,
if (I->getOpcode() == Instruction::Xor &&
       for (auto &R : Result)
         R.first = ConstantExpr::getNot(R.first);
 
       return true;
     }
 
   // Try to simplify some other binary operator values.
   } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(I)) {
-    assert(Preference != WantBlockAddress
-            && "A binary operator creating a block address?");
+    if (Preference != WantInteger)
+      return false;
     if (ConstantInt *CI = dyn_cast<ConstantInt>(BO->getOperand(1))) {
       PredValueInfoTy LHSVals;
       computeValueKnownInPredecessorsImpl(BO->getOperand(0), BB, LHSVals,
                                           WantInteger, RecursionSet, CxtI);
 
       // Try to use constant folding to simplify the binary operator.
       for (const auto &LHSVal : LHSVals) {
         Constant *V = LHSVal.first;
         Constant *Folded = ConstantExpr::get(BO->getOpcode(), V, CI);
 
         if (Constant *KC = getKnownConstant(Folded, WantInteger))
           Result.emplace_back(KC, LHSVal.second);
       }
     }
 
     return !Result.empty();
   }
 
   // Handle compare with phi operand, where the PHI is defined in this block.
   if (CmpInst *Cmp = dyn_cast<CmpInst>(I)) {
-    assert(Preference == WantInteger && "Compares only produce integers");
+    if (Preference != WantInteger)
+      return false;
     Type *CmpType = Cmp->getType();
     Value *CmpLHS = Cmp->getOperand(0);
     Value *CmpRHS = Cmp->getOperand(1);
     CmpInst::Predicate Pred = Cmp->getPredicate();
 
     PHINode *PN = dyn_cast<PHINode>(CmpLHS);
     if (!PN)
       PN = dyn_cast<PHINode>(CmpRHS);

llvm/test/Transforms/JumpThreading/indirectbr-cast-int-op.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -jump-threading -S < %s | FileCheck %s
+
+; The indirectbr needs a block address. The add can't produce that.
+; This shouldn't crash.
+define void @cast_with_binop() {
+; CHECK-LABEL: @cast_with_binop(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[OP:%.*]] = add i64 ptrtoint (i8* inttoptr (i32 1 to i8*) to i64), undef
+; CHECK-NEXT:    [[CAST:%.*]] = inttoptr i64 [[OP]] to i8*
+; CHECK-NEXT:    indirectbr i8* [[CAST]], [label [[BB3:%.*]], label %bb2]
+; CHECK:       bb2:
+; CHECK-NEXT:    unreachable
+; CHECK:       bb3:
+; CHECK-NEXT:    ret void
+;
+bb:
+  %op = add i64 ptrtoint (i8* inttoptr (i32 1 to i8*) to i64), undef
+  %cast = inttoptr i64 %op to i8*
+  indirectbr i8* %cast, [label %bb3, label %bb2]
+bb2:
+  unreachable
+bb3:
+  ret void
+}
+
+; The indirectbr needs a block address. The add can't produce that.
+; This shouldn't crash.
+define void @cast_with_i1(i1 %x, i1 %y) {
+; CHECK-LABEL: @cast_with_i1(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[OP:%.*]] = add i1 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[CAST:%.*]] = inttoptr i1 [[OP]] to i8*
+; CHECK-NEXT:    indirectbr i8* [[CAST]], [label [[BB3:%.*]], label %bb2]
+; CHECK:       bb2:
+; CHECK-NEXT:    unreachable
+; CHECK:       bb3:
+; CHECK-NEXT:    ret void
+;
+bb:
+  %op = add i1 %x, %y
+  %cast = inttoptr i1 %op to i8*
+  indirectbr i8* %cast, [label %bb3, label %bb2]
+bb2:
+  unreachable
+bb3:
+  ret void
+}
+
+; The indirectbr needs a block address. The cmp can't produce that.
+; This shouldn't crash.
+define void @cast_with_cmp(i1 %x, i1 %y) {
+; CHECK-LABEL: @cast_with_cmp(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[OP:%.*]] = icmp slt i1 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[CAST:%.*]] = inttoptr i1 [[OP]] to i8*
+; CHECK-NEXT:    indirectbr i8* [[CAST]], [label [[BB3:%.*]], label %bb2]
+; CHECK:       bb2:
+; CHECK-NEXT:    unreachable
+; CHECK:       bb3:
+; CHECK-NEXT:    ret void
+;
+bb:
+  %op = icmp slt i1 %x, %y
+  %cast = inttoptr i1 %op to i8*
+  indirectbr i8* %cast, [label %bb3, label %bb2]
+bb2:
+  unreachable
+bb3:
+  ret void
+}