[DA] getBackedgeTakenCount in isKnownLessThan can return CouldNotCompute

[sjoerdmeijer]

Bail out when the backedge taken count is a CouldNotCompute SCEV in function isKnownLessThan; we cannot and do not want to query things like its Type.

Fixes #159979

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Sjoerd Meijer (sjoerdmeijer)

Changes

Bail out when the backedge taken count is a CouldNotCompute SCEV in function isKnownLessThan; we cannot and do not want to query things like its Type.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/DependenceAnalysis.cpp (+2)
• (added) llvm/test/Analysis/DependenceAnalysis/becount-couldnotcompute.ll (+25)

diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 1f0da8d1830d3..9e474769abd83 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -1184,6 +1184,8 @@ bool DependenceInfo::isKnownLessThan(const SCEV *S, const SCEV *Size) const {
   if (const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(S))
     if (AddRec->isAffine() && AddRec->hasNoSignedWrap()) {
       const SCEV *BECount = SE->getBackedgeTakenCount(AddRec->getLoop());
+      if (isa<SCEVCouldNotCompute>(BECount))
+        return false;
       const SCEV *Start = AddRec->getStart();
       const SCEV *Step = AddRec->getStepRecurrence(*SE);
       const SCEV *End = AddRec->evaluateAtIteration(BECount, *SE);
diff --git a/llvm/test/Analysis/DependenceAnalysis/becount-couldnotcompute.ll b/llvm/test/Analysis/DependenceAnalysis/becount-couldnotcompute.ll
new file mode 100644
index 0000000000000..4828858897072
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/becount-couldnotcompute.ll
@@ -0,0 +1,25 @@
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 6
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"
+target triple = "aarch64-unknown-linux-gnu"
+
+; Test for function isKnownLessThan that calculates a back-edge taken count,
+; which can return a CouldNotCompute SCEV.
+
+define void @test(i64 %conv, ptr %a) {
+; CHECK-LABEL: 'test'
+; CHECK-NEXT:  Src: %.pre.pre.pre = load i32, ptr %arrayidx12, align 4 --> Dst: %.pre.pre.pre = load i32, ptr %arrayidx12, align 4
+; CHECK-NEXT:    da analyze - none!
+;
+entry:
+  %sub = add i64 %conv, 1
+  br label %for.cond
+
+for.cond:
+  %i.0 = phi i64 [ %add26, %for.cond ], [ 0, %entry ]
+  %arrayidx12 = getelementptr i32, ptr %a, i64 %i.0
+  %.pre.pre.pre = load i32, ptr %arrayidx12, align 4
+  %add26 = add nsw i64 %sub, %i.0
+  br label %for.cond
+}

[kasuga-fj]

Could you please clarify the motivation for this change? When I tried the test case you added, it crashed. So I guess this PR is meant to address that issue, right?

Side note: Although fixing such a bug is fine to me, I believe this function has multiple correctness issues and should be removed in the first place.

[kasuga-fj]

Suggested change

	target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"
	target triple = "aarch64-unknown-linux-gnu"

Maybe unnecessary?

[kasuga-fj]

I think this can be simplified. If you just want to write an infinite loop, some values (%i.0, %conv, etc.) should not be necessary.

[sjoerdmeijer]

Forgot to add that, but I have just updated the description, this fixes: #159979

[nikic]

Note that this is also going to skip the base case below.

[kasuga-fj]

It might be better to use DependenceInfo::collectUpperBound to obtain the BTC, as DA generally assumes the BTC to be loop-invariant with respect to the outermost loop when using it.

[sjoerdmeijer]

The test case is minimal to trigger the segfault, but I've removed DL and the triple, and renamed a few variables.
Instead of return false, I do now check the base case.

[kasuga-fj]

Generally speaking, I believe it's not a good idea to copy-and-paste the handling for basic case, as it's difficult to keep them consistent. In this case, I think something like the following would be better:

bool SpecialHandling = [&] {
  if (!AddRec->isAffine() || !AddRec->hasNoSignedWrap())
    return false;
  ...
  const SCEV *BECount = ...;
  if (!isa<SCEVCouldNotCompute>(BECount))
    return false;
  ...
}();

if (SpecialHandling)
  return true;

// Check using normal isKnownNegative
...