[SCEV] Check bitwidth for constant ranges comparison

[annamthomas]

Fix a crash if using isKnownPredicate with differing SCEV types.
We don't ever seem to assert that the types are the same and finally
crash when reaching the path of constant range comparisons.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: None (annamthomas)

Changes

Fix a crash if using isKnownPredicate with differing SCEV types.
We don't ever seem to assert that the types are the same and finally
crash when reaching the path of constant range comparisons.

---

Full diff: https://github.com/llvm/llvm-project/pull/150364.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+2-1)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 24adfa346c642..e535845c9a358 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -11380,7 +11380,8 @@ bool ScalarEvolution::isKnownPredicateViaConstantRanges(CmpPredicate Pred,
   auto CheckRange = [&](bool IsSigned) {
     auto RangeLHS = IsSigned ? getSignedRange(LHS) : getUnsignedRange(LHS);
     auto RangeRHS = IsSigned ? getSignedRange(RHS) : getUnsignedRange(RHS);
-    return RangeLHS.icmp(Pred, RangeRHS);
+    return RangeLHS.getBitWidth() == RangeRHS.getBitWidth() &&
+           RangeLHS.icmp(Pred, RangeRHS);
   };
 
   // The check at the top of the function catches the case where the values are

[annamthomas]

I think this should at least be an assert?

I used the isKnownPredicate downstream without converting the SCEV to the wider type and faced the crash when going through this API.

[nikic]

I think this should at least be an assert?

I used the isKnownPredicate downstream without converting the SCEV to the wider type and faced the crash when going through this API.

Yes, this should be an assert. These APIs generally assume matching types.

[annamthomas]

These APIs generally assume matching types.

Yep, I thought we would have that in the documentation of isKnownPredicate but didn't find it. I'll add the assert on SCEV types being the same in isKnownPredicateViaConstantRanges.

[fhahn]

Was this triggered by a caller passing mismatching types here that needs updating?

[annamthomas]

Was this triggered by a caller passing mismatching types here that needs updating?

Yep, basically I was adding a fast path downstream to evaluatePtrAddRecAtMaxBTCWillNotWrap API which uses isKnownPredicate (we have some information on java level loops). I needed to extend both SCEVs to the wider type. Without that, my change crashed here.

What is not clear to me is that we sometimes convert to wider types before using this API, but at other times we do not. Also, when I tried asserting for SCEV matching types in isKnownPredicateViaConstantRanges, there were lit test failures. So, I am not sure what the requirement of the passed-in SCEVs are.

I don't think we really need to add an assert at the RangeLHS.getBitWidth() == RangeRHS.getBitWidth() because it would anyway crash with the assert within the ConstantRange's utility.