[DA] Add check for base pointer invariance #148241

kasuga-fj · 2025-07-11T13:13:19Z

As specified in #53942, DA assumes base pointer invariance in its process. Some cases were fixed by #116628. However, that PR only addressed the parts related to AliasAnalysis, so the original issue persists in later stages, especially when the AliasAnalysis results in MustAlias.
This patch insert an explicit loop-invariant checks for the base pointer and skips analysis when it is not loop-invariant.

Fix the cases added in #148240.

llvmbot · 2025-07-11T13:13:51Z

@llvm/pr-subscribers-llvm-analysis

Author: Ryotaro Kasuga (kasuga-fj)

Changes

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

2 Files Affected:

(modified) llvm/lib/Analysis/DependenceAnalysis.cpp (+16)
(modified) llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll (+9-9)

diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 428342f51ad2e..07bf560772c8c 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -3383,6 +3383,10 @@ bool DependenceInfo::tryDelinearize(Instruction *Src, Instruction *Dst,
                                     SrcSubscripts, DstSubscripts))
     return false;
 
+  assert(isLoopInvariant(SrcBase, SrcLoop) &&
+         isLoopInvariant(DstBase, DstLoop) &&
+         "Expected SrcBase and DstBase to be loop invariant");
+
   int Size = SrcSubscripts.size();
   LLVM_DEBUG({
     dbgs() << "\nSrcSubscripts: ";
@@ -3666,6 +3670,18 @@ DependenceInfo::depends(Instruction *Src, Instruction *Dst,
                                         SCEVUnionPredicate(Assume, *SE));
   }
 
+  // Even if the base pointers are the same, they may not be loop-invariant. It
+  // could lead to incorrect results, as we're analyzing loop-carried
+  // dependencies.
+  Loop *SrcLoop = LI->getLoopFor(Src->getParent());
+  Loop *DstLoop = LI->getLoopFor(Dst->getParent());
+  if (!isLoopInvariant(SrcBase, SrcLoop) ||
+      !isLoopInvariant(DstBase, DstLoop)) {
+    LLVM_DEBUG(dbgs() << "The base pointer is not loop invariant.\n");
+    return std::make_unique<Dependence>(Src, Dst,
+                                        SCEVUnionPredicate(Assume, *SE));
+  }
+
   uint64_t EltSize = SrcLoc.Size.toRaw();
   const SCEV *SrcEv = SE->getMinusSCEV(SrcSCEV, SrcBase);
   const SCEV *DstEv = SE->getMinusSCEV(DstSCEV, DstBase);
diff --git a/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll b/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll
index 843c18a6e0d1e..a357018563be1 100644
--- a/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll
@@ -8,11 +8,11 @@
 define float @bug41488_test1(float %f) {
 ; CHECK-LABEL: 'bug41488_test1'
 ; CHECK-NEXT:  Src: %0 = load float, ptr %p, align 4 --> Dst: %0 = load float, ptr %p, align 4
-; CHECK-NEXT:    da analyze - input [*]!
+; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: %0 = load float, ptr %p, align 4 --> Dst: store float %f, ptr %q, align 4
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store float %f, ptr %q, align 4 --> Dst: store float %f, ptr %q, align 4
-; CHECK-NEXT:    da analyze - output [*]!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   %g = alloca float, align 4
@@ -34,11 +34,11 @@ for.cond.cleanup:
 define void @bug41488_test2(i32 %n) {
 ; CHECK-LABEL: 'bug41488_test2'
 ; CHECK-NEXT:  Src: %0 = load float, ptr %p, align 4 --> Dst: %0 = load float, ptr %p, align 4
-; CHECK-NEXT:    da analyze - input [*]!
+; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: %0 = load float, ptr %p, align 4 --> Dst: store float 0.000000e+00, ptr %q, align 4
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store float 0.000000e+00, ptr %q, align 4 --> Dst: store float 0.000000e+00, ptr %q, align 4
-; CHECK-NEXT:    da analyze - output [*]!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   %g = alloca float, align 4
@@ -68,7 +68,7 @@ define void @bug53942_foo(i32 noundef %n, ptr noalias nocapture noundef writeonl
 ; CHECK-NEXT:  Src: %.pre = load double, ptr %B, align 8 --> Dst: store double %.pre, ptr %arrayidx2, align 8
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store double %.pre, ptr %arrayidx2, align 8 --> Dst: store double %.pre, ptr %arrayidx2, align 8
-; CHECK-NEXT:    da analyze - output [*]!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   %cmp8 = icmp sgt i32 %n, 1
@@ -99,11 +99,11 @@ for.body:                                         ; preds = %for.body.preheader,
 define void @bug53942_bar(i32 noundef %n, ptr noalias noundef %A, ptr noalias noundef %B) {
 ; CHECK-LABEL: 'bug53942_bar'
 ; CHECK-NEXT:  Src: %0 = load double, ptr %arrayidx, align 8 --> Dst: %0 = load double, ptr %arrayidx, align 8
-; CHECK-NEXT:    da analyze - input [*]!
+; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: %0 = load double, ptr %arrayidx, align 8 --> Dst: store double %0, ptr %arrayidx8, align 8
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store double %0, ptr %arrayidx8, align 8 --> Dst: store double %0, ptr %arrayidx8, align 8
-; CHECK-NEXT:    da analyze - output [*]!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   br label %for.cond
@@ -173,7 +173,7 @@ for.end:                                          ; preds = %for.cond.cleanup
 define void @non_invariant_baseptr_with_identical_obj(ptr %A) {
 ; CHECK-LABEL: 'non_invariant_baseptr_with_identical_obj'
 ; CHECK-NEXT:  Src: store i32 1, ptr %idx, align 4 --> Dst: store i32 1, ptr %idx, align 4
-; CHECK-NEXT:    da analyze - none!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   br label %loop.i.header
@@ -222,7 +222,7 @@ exit:
 define void @non_invariant_baseptr_with_identical_underlying_obj2(ptr %A) {
 ; CHECK-LABEL: 'non_invariant_baseptr_with_identical_underlying_obj2'
 ; CHECK-NEXT:  Src: store i32 1, ptr %idx, align 4 --> Dst: store i32 1, ptr %idx, align 4
-; CHECK-NEXT:    da analyze - none!
+; CHECK-NEXT:    da analyze - confused!
 ;
 entry:
   br label %loop.i.header

kasuga-fj · 2025-07-11T18:01:14Z

llvm/test/Analysis/DependenceAnalysis/Banerjee.ll

@@ -113,7 +113,7 @@ define void @banerjee1(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
 ; CHECK-NEXT:  Src: %2 = load i64, ptr %arrayidx6, align 8 --> Dst: store i64 %2, ptr %B.addr.12, align 8
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store i64 %2, ptr %B.addr.12, align 8 --> Dst: store i64 %2, ptr %B.addr.12, align 8
-; CHECK-NEXT:    da analyze - output [* *]!


All the test changes except for FlipFlopBaseAddress.ll are related to dependencies between memory accesses of the form *B++.

*B++ should be fine though, right? Should the condition possibly be loop invariant or addrec for the appropriate loop?

*B++ should be fine though, right?

Yes, I think.

Should the condition possibly be loop invariant or addrec for the appropriate loop?

Probably yes, but I'm not quite sure what qualifies as an "appropriate loop". In this case, the SCEV value for %B.addr.12 is {%B.addr.06,+,8}<nuw><%for.body3> rather than something like {{%B,+,(8 * %m)}<%for.cond1.preheader>,+,8}<%for.body3>. Can we infer anything from the former SCEV representation?

Add test cases where DA yields incorrect results because it makes an assumption that the base pointer is loop-invariant, which doesn't hold in these cases. Will be fixed by #148241.

nikic

Does the remaining issue occur only with an access having a dependence on itself?

kasuga-fj

Does the remaining issue occur only with an access having a dependence on itself?

Added another test in 1df731b. Are you considering cases like this? If so, then the answer is YES. The dependencies between two distinct stores can be missed as well (godbolt: https://godbolt.org/z/7Mhb9rzGc).

kasuga-fj · 2025-07-17T11:00:37Z

llvm/test/Analysis/DependenceAnalysis/Banerjee.ll

@@ -113,7 +113,7 @@ define void @banerjee1(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
 ; CHECK-NEXT:  Src: %2 = load i64, ptr %arrayidx6, align 8 --> Dst: store i64 %2, ptr %B.addr.12, align 8
 ; CHECK-NEXT:    da analyze - confused!
 ; CHECK-NEXT:  Src: store i64 %2, ptr %B.addr.12, align 8 --> Dst: store i64 %2, ptr %B.addr.12, align 8
-; CHECK-NEXT:    da analyze - output [* *]!


*B++ should be fine though, right?

Yes, I think.

Should the condition possibly be loop invariant or addrec for the appropriate loop?

Probably yes, but I'm not quite sure what qualifies as an "appropriate loop". In this case, the SCEV value for %B.addr.12 is {%B.addr.06,+,8}<nuw><%for.body3> rather than something like {{%B,+,(8 * %m)}<%for.cond1.preheader>,+,8}<%for.body3>. Can we infer anything from the former SCEV representation?

kasuga-fj · 2025-07-25T11:20:39Z

Gentle ping

sjoerdmeijer

Looks like a good fix to me.
Please wait a day before merging this in case @nikic has more comments.

nikic

I'm okay with this, as it's conservatively correct. We can try to support more cases in a safe way later.

As specified in llvm#53942, DA assumes base pointer invariance in its process. Some cases were fixed by llvm#116628. However, that PR only addressed the parts related to AliasAnalysis, so the original issue persists in later stages, especially when the AliasAnalysis results in `MustAlias`. This patch insert an explicit loop-invariant checks for the base pointer and skips analysis when it is not loop-invariant. Fix the cases added in llvm#148240.

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jul 11, 2025

kasuga-fj mentioned this pull request Jul 11, 2025

[DA] Add test cases where base ptr is not loop-invariant #148240

Merged

kasuga-fj force-pushed the users/kasuga-fj/da-check-baseptr-0 branch from 0b6d75d to 00e9a7d Compare July 11, 2025 13:25

kasuga-fj force-pushed the users/kasuga-fj/da-check-baseptr-1 branch from 3596b17 to d914ea5 Compare July 11, 2025 13:25

kasuga-fj requested review from nikic, sebpop and sjoerdmeijer July 11, 2025 13:56

kasuga-fj marked this pull request as draft July 11, 2025 16:18

kasuga-fj commented Jul 11, 2025

View reviewed changes

kasuga-fj marked this pull request as ready for review July 11, 2025 18:20

Base automatically changed from users/kasuga-fj/da-check-baseptr-0 to main July 14, 2025 01:51

[DA] Add check for base pointer invariance

4977f6f

kasuga-fj force-pushed the users/kasuga-fj/da-check-baseptr-1 branch from b10a5ab to 4977f6f Compare July 14, 2025 01:56

kasuga-fj mentioned this pull request Jul 15, 2025

[Delinearization] Add function for fixed size array without relying on GEP #145050

Open

nikic reviewed Jul 17, 2025

View reviewed changes

Add test

1df731b

kasuga-fj commented Jul 17, 2025

View reviewed changes

sjoerdmeijer approved these changes Jul 25, 2025

View reviewed changes

nikic approved these changes Jul 25, 2025

View reviewed changes

kasuga-fj merged commit b06f10d into main Jul 25, 2025
9 checks passed

kasuga-fj deleted the users/kasuga-fj/da-check-baseptr-1 branch July 25, 2025 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DA] Add check for base pointer invariance #148241

[DA] Add check for base pointer invariance #148241

Uh oh!

kasuga-fj commented Jul 11, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 11, 2025

Uh oh!

kasuga-fj Jul 11, 2025

Uh oh!

nikic Jul 17, 2025

Uh oh!

kasuga-fj Jul 17, 2025

Uh oh!

nikic left a comment

Uh oh!

kasuga-fj left a comment

Uh oh!

kasuga-fj Jul 17, 2025

Uh oh!

kasuga-fj commented Jul 25, 2025

Uh oh!

sjoerdmeijer left a comment

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

[DA] Add check for base pointer invariance #148241

[DA] Add check for base pointer invariance #148241

Uh oh!

Conversation

kasuga-fj commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 11, 2025

Uh oh!

kasuga-fj Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

nikic Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj commented Jul 25, 2025

Uh oh!

sjoerdmeijer left a comment

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kasuga-fj commented Jul 11, 2025 •

edited

Loading