[ScalarEvolution] Add bailout to avoid zext of pointer.

[efriedma]

Summary

The RHS of an isImpliedCond call can be a pointer even if the LHS is not. This is similar to bfa2a81e.

Not going to include a testcase; an IR testcase would be extremely complicated and fragile.

Fixes https://bugs.llvm.org/show_bug.cgi?id=52594 .

[dim]

FWIW, this fixes both the minimized test case from PR52594, and the full original test case from https://bugs.freebsd.org/260000.

[fhahn]

I'm wondering about:

Not going to include a testcase; an IR testcase would be extremely complicated and fragile.

Below a reduced version from PR52594, which doesn't look too bad IMO. Is there anything in particular you think makes this test very fragile>

target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"

define void @test(i8* %a, i8* %b, i64 %c) {
entry:
  br label %loop.1.header

loop.1.header:
  %iv.1 = phi i8* [ %a, %entry ], [ %iv.1.next, %loop.1.latch ]
  %cond = icmp eq i64 %c, 0
  br i1 %cond, label %loop.1.latch, label %exit

loop.1.latch:
  %iv.1.next = getelementptr inbounds i8, i8* %iv.1, i32 1
  %ec = icmp eq i8* %iv.1.next, %b
  br i1 %ec, label %loop.2.ph, label %loop.1.header

loop.2.ph:
  br label %loop.2

loop.2:
  %iv.2 = phi i8* [ %iv.1.next, %loop.2.ph ], [ %iv.2.next, %loop.2 ]
  %iv.2.int = ptrtoint i8* %iv.2 to i32
  call void @wobble(i32 %iv.2.int)
  %ec.1 = icmp ult i8* %iv.2, %b
  %ec.1.trunc = and i1 %ec.1, 1
  %iv.2.next = getelementptr inbounds i8, i8* %iv.2, i32 1
  br i1 %ec.1.trunc, label %loop.2, label %exit

exit:
  ret void
}

declare void @wobble(i32)

llvm/lib/Analysis/ScalarEvolution.cpp

10941

can this be moved up before if (!CmpInst::isSigned(FoundPred).... and the pointer check dropped from the condition above?

[mkazantsev]

llvm/lib/Analysis/ScalarEvolution.cpp

10952

This doesn't make sense to me. I'd expect that both FoundLHS and FoundRHS should be of the same type. How come they are different? Isn't it the real reason of the bug?

[efriedma]

The testcase isn't that complicated in terms of the size of the IR, but it would be hard to ensure it continues to test the codepath in question. The actual crash involves many levels of recursive calls in SCEV, and we aren't expecting any useful information out of the isImpliedCond analysis in this case. I could include it anyway, I guess.

llvm/lib/Analysis/ScalarEvolution.cpp

10952

Like we found doing the review for bfa2a81e, the pointer-ness of the LHS/RHS should match for an icmp instruction, but ScalarEvolution makes other calls recursively. See, for example, ScalarEvolution::isImpliedCondOperandsHelper.

At one point, I was considering using ptrtoint on all pointer inputs to isImpliedCond, but that turned out to be a little more complicated than I expected, and not really necessary. I could revive that, I guess.

[efriedma]

llvm/lib/Analysis/ScalarEvolution.cpp

10941

This check is checking FoundLHS/FoundRHS; the if (!CmpInst::isSigned(FoundPred).... check is checking LHS/RHS.

[reames]

LGTM

llvm/lib/Analysis/ScalarEvolution.cpp

bool ScalarEvolution::isImpliedCond(ICmpInst::Predicate Pred, const SCEV *LHS,
                                     const SCEV *FoundLHS, const SCEV *FoundRHS,
                                     const Instruction *CtxI) {
   // Balance the types.
   if (getTypeSizeInBits(LHS->getType()) <
       getTypeSizeInBits(FoundLHS->getType())) {
     // For unsigned and equality predicates, try to prove that both found
     // operands fit into narrow unsigned range. If so, try to prove facts in
     // narrow types.
-    if (!CmpInst::isSigned(FoundPred) && !FoundLHS->getType()->isPointerTy()) {
+    if (!CmpInst::isSigned(FoundPred) && !FoundLHS->getType()->isPointerTy() &&
+        !FoundRHS->getType()->isPointerTy()) {
       auto *NarrowType = LHS->getType();
       auto *WideType = FoundLHS->getType();
       auto BitWidth = getTypeSizeInBits(NarrowType);
       const SCEV *MaxValue = getZeroExtendExpr(
           getConstant(APInt::getMaxValue(BitWidth)), WideType);
       if (isKnownViaNonRecursiveReasoning(ICmpInst::ICMP_ULE, FoundLHS,
                                           MaxValue) &&
           isKnownViaNonRecursiveReasoning(ICmpInst::ICMP_ULE, FoundRHS,
                                           MaxValue)) {
         const SCEV *TruncFoundLHS = getTruncateExpr(FoundLHS, NarrowType);
         const SCEV *TruncFoundRHS = getTruncateExpr(FoundRHS, NarrowType);
         if (isImpliedCondBalancedTypes(Pred, LHS, RHS, FoundPred, TruncFoundLHS,
                                        TruncFoundRHS, CtxI))
           return true;
       }
     }
 
-    if (LHS->getType()->isPointerTy())
+    if (LHS->getType()->isPointerTy() || RHS->getType()->isPointerTy())
       return false;
     if (CmpInst::isSigned(Pred)) {
       LHS = getSignExtendExpr(LHS, FoundLHS->getType());
       RHS = getSignExtendExpr(RHS, FoundLHS->getType());
     } else {
       LHS = getZeroExtendExpr(LHS, FoundLHS->getType());
       RHS = getZeroExtendExpr(RHS, FoundLHS->getType());
     }
   } else if (getTypeSizeInBits(LHS->getType()) >
       getTypeSizeInBits(FoundLHS->getType())) {
-    if (FoundLHS->getType()->isPointerTy())
+    if (FoundLHS->getType()->isPointerTy() || FoundRHS->getType()->isPointerTy())
       return false;
     if (CmpInst::isSigned(FoundPred)) {
       FoundLHS = getSignExtendExpr(FoundLHS, LHS->getType());
       FoundRHS = getSignExtendExpr(FoundRHS, LHS->getType());
     } else {
       FoundLHS = getZeroExtendExpr(FoundLHS, LHS->getType());
       FoundRHS = getZeroExtendExpr(FoundRHS, LHS->getType());
     }