[LAA] Clean up APInt-overflow related code

[artagnon]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

Use APInt::{trySExtValue,tryZExtValue} instead of the asserting variants. The change is untestable, as the test demonstrates, since getTypeSizeInBits is not equipped to handle type-sizes over 64-bits, and the constant distance flowing through LAA is -1 as a consequence.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/LoopAccessAnalysis.cpp (+26-24)
• (added) llvm/test/Analysis/LoopAccessAnalysis/long-pointer-distance.ll (+37)

diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index ab407e945bc53..63eb97eaeca5b 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -827,16 +827,13 @@ getStrideFromAddRec(const SCEVAddRecExpr *AR, const Loop *Lp, Type *AccessTy,
   TypeSize AllocSize = DL.getTypeAllocSize(AccessTy);
   int64_t Size = AllocSize.getFixedValue();
 
-  // Huge step value - give up.
-  if (APStepVal->getBitWidth() > 64)
+  std::optional<int64_t> StepVal = APStepVal->trySExtValue();
+  if (!StepVal)
     return std::nullopt;
 
-  int64_t StepVal = APStepVal->getSExtValue();
-
   // Strided access.
-  int64_t Stride = StepVal / Size;
-  int64_t Rem = StepVal % Size;
-  if (Rem)
+  int64_t Stride = *StepVal / Size;
+  if (*StepVal % Size)
     return std::nullopt;
 
   return Stride;
@@ -2067,14 +2064,17 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
     return Dependence::NoDep;
 
   // Attempt to prove strided accesses independent.
-  const APInt *ConstDist = nullptr;
-  if (match(Dist, m_scev_APInt(ConstDist))) {
-    uint64_t Distance = ConstDist->abs().getZExtValue();
+  const APInt *APDist = nullptr;
+  std::optional<uint64_t> ConstDistance = match(Dist, m_scev_APInt(APDist))
+                                              ? APDist->abs().tryZExtValue()
+                                              : std::nullopt;
 
+  if (ConstDistance) {
     // If the distance between accesses and their strides are known constants,
     // check whether the accesses interlace each other.
-    if (Distance > 0 && CommonStride && CommonStride > 1 && HasSameSize &&
-        areStridedAccessesIndependent(Distance, *CommonStride, TypeByteSize)) {
+    if (ConstDistance > 0 && CommonStride && CommonStride > 1 && HasSameSize &&
+        areStridedAccessesIndependent(*ConstDistance, *CommonStride,
+                                      TypeByteSize)) {
       LLVM_DEBUG(dbgs() << "LAA: Strided accesses are independent\n");
       return Dependence::NoDep;
     }
@@ -2107,7 +2107,7 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
     // forward dependency will allow vectorization using any width.
 
     if (IsTrueDataDependence && EnableForwardingConflictDetection) {
-      if (!ConstDist) {
+      if (!ConstDistance) {
         // TODO: FoundNonConstantDistanceDependence is used as a necessary
         // condition to consider retrying with runtime checks. Historically, we
         // did not set it when strides were different but there is no inherent
@@ -2115,8 +2115,8 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
         FoundNonConstantDistanceDependence |= ShouldRetryWithRuntimeCheck;
         return Dependence::Unknown;
       }
-      if (!HasSameSize || couldPreventStoreLoadForward(
-                              ConstDist->abs().getZExtValue(), TypeByteSize)) {
+      if (!HasSameSize ||
+          couldPreventStoreLoadForward(*ConstDistance, TypeByteSize)) {
         LLVM_DEBUG(
             dbgs() << "LAA: Forward but may prevent st->ld forwarding\n");
         return Dependence::ForwardButPreventsForwarding;
@@ -2127,14 +2127,15 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
     return Dependence::Forward;
   }
 
-  int64_t MinDistance = SE.getSignedRangeMin(Dist).getSExtValue();
+  std::optional<int64_t> MinDistance =
+      SE.getSignedRangeMin(Dist).trySExtValue();
   // Below we only handle strictly positive distances.
-  if (MinDistance <= 0) {
+  if (!MinDistance || MinDistance <= 0) {
     FoundNonConstantDistanceDependence |= ShouldRetryWithRuntimeCheck;
     return Dependence::Unknown;
   }
 
-  if (!ConstDist) {
+  if (!ConstDistance) {
     // Previously this case would be treated as Unknown, possibly setting
     // FoundNonConstantDistanceDependence to force re-trying with runtime
     // checks. Until the TODO below is addressed, set it here to preserve
@@ -2193,8 +2194,8 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
   // minimum for computations below, as this ensures we compute the closest
   // possible dependence distance.
   uint64_t MinDistanceNeeded = MaxStride * (MinNumIter - 1) + TypeByteSize;
-  if (MinDistanceNeeded > static_cast<uint64_t>(MinDistance)) {
-    if (!ConstDist) {
+  if (MinDistanceNeeded > static_cast<uint64_t>(*MinDistance)) {
+    if (!ConstDistance) {
       // For non-constant distances, we checked the lower bound of the
       // dependence distance and the distance may be larger at runtime (and safe
       // for vectorization). Classify it as Unknown, so we re-try with runtime
@@ -2231,11 +2232,12 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
   // is 8, which is less than 2 and forbidden vectorization, But actually
   // both A and B could be vectorized by 2 iterations.
   MinDepDistBytes =
-      std::min(static_cast<uint64_t>(MinDistance), MinDepDistBytes);
+      std::min(static_cast<uint64_t>(*MinDistance), MinDepDistBytes);
 
   bool IsTrueDataDependence = (!AIsWrite && BIsWrite);
-  if (IsTrueDataDependence && EnableForwardingConflictDetection && ConstDist &&
-      couldPreventStoreLoadForward(MinDistance, TypeByteSize, *CommonStride))
+  if (IsTrueDataDependence && EnableForwardingConflictDetection &&
+      ConstDistance &&
+      couldPreventStoreLoadForward(*MinDistance, TypeByteSize, *CommonStride))
     return Dependence::BackwardVectorizableButPreventsForwarding;
 
   uint64_t MaxVF = MinDepDistBytes / MaxStride;
@@ -2243,7 +2245,7 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
                     << " with max VF = " << MaxVF << '\n');
 
   uint64_t MaxVFInBits = MaxVF * TypeByteSize * 8;
-  if (!ConstDist && MaxVFInBits < MaxTargetVectorWidthInBits) {
+  if (!ConstDistance && MaxVFInBits < MaxTargetVectorWidthInBits) {
     // For non-constant distances, we checked the lower bound of the dependence
     // distance and the distance may be larger at runtime (and safe for
     // vectorization). Classify it as Unknown, so we re-try with runtime checks.
diff --git a/llvm/test/Analysis/LoopAccessAnalysis/long-pointer-distance.ll b/llvm/test/Analysis/LoopAccessAnalysis/long-pointer-distance.ll
new file mode 100644
index 0000000000000..094d2a0e7fa37
--- /dev/null
+++ b/llvm/test/Analysis/LoopAccessAnalysis/long-pointer-distance.ll
@@ -0,0 +1,37 @@
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes='print<access-info>' -disable-output %s 2>&1 | FileCheck %s
+
+define void @test(ptr %this, i128 %loop.limit) {
+; CHECK-LABEL: 'test'
+; CHECK-NEXT:    loop:
+; CHECK-NEXT:      Memory dependences are safe
+; CHECK-NEXT:      Dependences:
+; CHECK-NEXT:        Forward:
+; CHECK-NEXT:            store i64 1, ptr %c, align 8 ->
+; CHECK-NEXT:            store i64 2, ptr %d, align 8
+; CHECK-EMPTY:
+; CHECK-NEXT:      Run-time memory checks:
+; CHECK-NEXT:      Grouped accesses:
+; CHECK-EMPTY:
+; CHECK-NEXT:      Non vectorizable stores to invariant address were not found in loop.
+; CHECK-NEXT:      SCEV assumptions:
+; CHECK-EMPTY:
+; CHECK-NEXT:      Expressions re-written:
+;
+entry:
+  br label %loop
+loop:
+  %iv = phi i128 [ 0, %entry ], [ %iv.next, %loop ]
+  %iv.mul.8 = mul i128 %iv, 8
+  %c = getelementptr i8, ptr %this, i128 %iv.mul.8
+  store i64 1, ptr %c, align 8
+  %iv.off = add i128 %iv.mul.8, u0x00FFFFFFFFFFFFFFFF
+  %d = getelementptr i8, ptr %this, i128 %iv.off
+  store i64 2, ptr %d, align 8
+  %iv.next = add nuw nsw i128 %iv, 1
+  %exit.cond = icmp ult i128 %iv.next, %loop.limit
+  br i1 %exit.cond, label %loop, label %exit
+exit:
+  ret void
+}
+

[artagnon]

I thought about this PR some more, and I don't think the tests add value: I've dropped the tests, and this should be a pure NFC cleanup now.

[fhahn]

LGTM, thanks