Bug fixes for Array Dependence Analysis #116389
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This patch fixes two bugs: llvm#41488 llvm#53942 The dependence analysis assumes that the base address of array accesses is invariant across loop iterations. In both bugs the base address evolves following loop iterations: the base address flip-flops between two different memory objects. Based on the scalar evolution of base addresses, the patch adds code to separate the 3 alias cases {must, no, may}-alias where the base addresses are identical at every iteration, never the same, and unknown.
…sizes This fixes bug llvm#16183 where the elements of a same array are accesses as i32 and i64. This is not handled by the array dependence analysis.
Make sure the testcase for this bug continues to work: llvm#31196
DA uses SCEV to solve linear constraints. When it generates SCEVs with negative
strides, i.e., {0,+,-1}, make sure the SCEVs are marked as non wrap arithmetic.
This patch fixes llvm#51512
|
Did you consider submitting the 4 patches as 4 different PRs? If they depend on each other, see https://llvm.org/docs/GitHub.html#branches |
|
The patches do not depend on each other and can be submitted as separate PRs. |
|
@llvm/pr-subscribers-llvm-analysis Author: Sebastian Pop (sebpop) ChangesThe Array Dependence Analysis (DA) was built based on several assumptions (mostly coming from properties guaranteed by the Fortran language.) DA needs to conservatively answer "don't know" when the assumptions are not met. This patch series adds code to detect some of the assumptions. The first patch [DA] disambiguate evolution of base addresses ensures that the base address is invariant in DA. The patch uses SCEV analysis. This fixes two bugs: #41488 and #53942 The second patch [DA] Dependence analysis does not handle array accesses of different sizes makes sure all memory accesses have the same byte width. This fixes bug #16183 The third patch [DA] add testcase adds a missing test for a bug that seems to be already fixed: #31196 The fourth patch [DA] use NSW arithmetic fixes #51512 where the constraints solver in DA was using wrap-around instead of NSW arithmetic. The patch series was tested on arm64-linux. Full diff: https://github.com/llvm/llvm-project/pull/116389.diff 5 Files Affected:
diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index a4a98ea0bae146..5e1aa523923b02 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -712,10 +712,66 @@ void Dependence::dump(raw_ostream &OS) const {
// tbaa, non-overlapping regions etc), then it is known there is no dependecy.
// Otherwise the underlying objects are checked to see if they point to
// different identifiable objects.
-static AliasResult underlyingObjectsAlias(AAResults *AA,
- const DataLayout &DL,
- const MemoryLocation &LocA,
- const MemoryLocation &LocB) {
+static AliasResult underlyingObjectsAlias(AAResults *AA, LoopInfo *LI,
+ ScalarEvolution *SE, Instruction *A,
+ Instruction *B) {
+ const MemoryLocation &LocA = MemoryLocation::get(A);
+ const MemoryLocation &LocB = MemoryLocation::get(B);
+
+ // Check the underlying objects are the same
+ const Value *AObj = getUnderlyingObject(LocA.Ptr);
+ const Value *BObj = getUnderlyingObject(LocB.Ptr);
+
+ if (AObj == BObj) {
+ // The dependence test gets confused if the size of the memory accesses
+ // differ.
+ if (LocA.Size != LocB.Size)
+ return AliasResult::MayAlias;
+
+ // If the underlying objects are the same, they must alias.
+ return AliasResult::MustAlias;
+ }
+
+ if (auto *APhi = dyn_cast<PHINode>(AObj)) {
+ if (auto *BPhi = dyn_cast<PHINode>(BObj)) {
+ Loop *ALoop = LI->getLoopFor(APhi->getParent());
+ Loop *BLoop = LI->getLoopFor(BPhi->getParent());
+ if (ALoop == BLoop) {
+ auto *SCEVa = SE->getSCEV(const_cast<Value *>(AObj));
+ auto *SCEVb = SE->getSCEV(const_cast<Value *>(BObj));
+
+ // If the SCEVs are the same, they must alias.
+ if (SCEVa == SCEVb)
+ return AliasResult::MustAlias;
+
+ // If SCEV cannot analyze one of the values, then they may alias.
+ if (isa<SCEVUnknown>(SCEVa) || isa<SCEVUnknown>(SCEVb))
+ return AliasResult::MayAlias;
+
+ // Check whether the start values alias.
+ const SCEV *StartA = SCEVa;
+ while (const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(StartA))
+ StartA = AR->getStart();
+
+ const SCEV *StartB = SCEVb;
+ while (const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(StartB))
+ StartB = AR->getStart();
+
+ if (const SCEVUnknown *UA = dyn_cast<SCEVUnknown>(StartA)) {
+ if (const SCEVUnknown *UB = dyn_cast<SCEVUnknown>(StartB)) {
+ MemoryLocation LocAS =
+ MemoryLocation::getBeforeOrAfter(UA->getValue());
+ MemoryLocation LocBS =
+ MemoryLocation::getBeforeOrAfter(UB->getValue());
+ if (AA->isNoAlias(LocAS, LocBS))
+ return AliasResult::NoAlias;
+ }
+ }
+ return AliasResult::MayAlias;
+ }
+ }
+ }
+
// Check the original locations (minus size) for noalias, which can happen for
// tbaa, incompatible underlying object locations, etc.
MemoryLocation LocAS =
@@ -725,14 +781,6 @@ static AliasResult underlyingObjectsAlias(AAResults *AA,
if (AA->isNoAlias(LocAS, LocBS))
return AliasResult::NoAlias;
- // Check the underlying objects are the same
- const Value *AObj = getUnderlyingObject(LocA.Ptr);
- const Value *BObj = getUnderlyingObject(LocB.Ptr);
-
- // If the underlying objects are the same, they must alias
- if (AObj == BObj)
- return AliasResult::MustAlias;
-
// We may have hit the recursion limit for underlying objects, or have
// underlying objects where we don't know they will alias.
if (!isIdentifiedObject(AObj) || !isIdentifiedObject(BObj))
@@ -743,7 +791,6 @@ static AliasResult underlyingObjectsAlias(AAResults *AA,
return AliasResult::NoAlias;
}
-
// Returns true if the load or store can be analyzed. Atomic and volatile
// operations have properties which this analysis does not understand.
static
@@ -3116,11 +3163,9 @@ const SCEV *DependenceInfo::addToCoefficient(const SCEV *Expr,
const Loop *TargetLoop,
const SCEV *Value) const {
const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Expr);
- if (!AddRec) // create a new addRec
- return SE->getAddRecExpr(Expr,
- Value,
- TargetLoop,
- SCEV::FlagAnyWrap); // Worst case, with no info.
+ if (!AddRec)
+ return SE->getAddRecExpr(Expr, Value, TargetLoop, SCEV::FlagNSW);
+
if (AddRec->getLoop() == TargetLoop) {
const SCEV *Sum = SE->getAddExpr(AddRec->getStepRecurrence(*SE), Value);
if (Sum->isZero())
@@ -3131,7 +3176,7 @@ const SCEV *DependenceInfo::addToCoefficient(const SCEV *Expr,
AddRec->getNoWrapFlags());
}
if (SE->isLoopInvariant(AddRec, TargetLoop))
- return SE->getAddRecExpr(AddRec, Value, TargetLoop, SCEV::FlagAnyWrap);
+ return SE->getAddRecExpr(AddRec, Value, TargetLoop, SCEV::FlagNSW);
return SE->getAddRecExpr(
addToCoefficient(AddRec->getStart(), TargetLoop, Value),
AddRec->getStepRecurrence(*SE), AddRec->getLoop(),
@@ -3606,9 +3651,7 @@ DependenceInfo::depends(Instruction *Src, Instruction *Dst,
Value *SrcPtr = getLoadStorePointerOperand(Src);
Value *DstPtr = getLoadStorePointerOperand(Dst);
- switch (underlyingObjectsAlias(AA, F->getDataLayout(),
- MemoryLocation::get(Dst),
- MemoryLocation::get(Src))) {
+ switch (underlyingObjectsAlias(AA, LI, SE, Dst, Src)) {
case AliasResult::MayAlias:
case AliasResult::PartialAlias:
// cannot analyse objects if we don't understand their aliasing.
@@ -4030,11 +4073,8 @@ const SCEV *DependenceInfo::getSplitIteration(const Dependence &Dep,
assert(Dst->mayReadFromMemory() || Dst->mayWriteToMemory());
assert(isLoadOrStore(Src));
assert(isLoadOrStore(Dst));
- Value *SrcPtr = getLoadStorePointerOperand(Src);
- Value *DstPtr = getLoadStorePointerOperand(Dst);
- assert(underlyingObjectsAlias(
- AA, F->getDataLayout(), MemoryLocation::get(Dst),
- MemoryLocation::get(Src)) == AliasResult::MustAlias);
+ assert(underlyingObjectsAlias(AA, LI, SE, Dst, Src) ==
+ AliasResult::MustAlias);
// establish loop nesting levels
establishNestingLevels(Src, Dst);
@@ -4043,6 +4083,8 @@ const SCEV *DependenceInfo::getSplitIteration(const Dependence &Dep,
unsigned Pairs = 1;
SmallVector<Subscript, 2> Pair(Pairs);
+ Value *SrcPtr = getLoadStorePointerOperand(Src);
+ Value *DstPtr = getLoadStorePointerOperand(Dst);
const SCEV *SrcSCEV = SE->getSCEV(SrcPtr);
const SCEV *DstSCEV = SE->getSCEV(DstPtr);
Pair[0].Src = SrcSCEV;
diff --git a/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll b/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll
new file mode 100644
index 00000000000000..4d5da005ea437b
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/DifferentAccessSize.ll
@@ -0,0 +1,15 @@
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: | FileCheck %s
+
+; The dependence test does not handle array accesses of different sizes: i32 and i64.
+; Bug 16183 - https://github.com/llvm/llvm-project/issues/16183
+; CHECK-LABEL: bug16183_alias
+; CHECK: da analyze - confused!
+
+define i64 @bug16183_alias(i32* nocapture %A) {
+entry:
+ %arrayidx = getelementptr inbounds i32, ptr %A, i64 1
+ store i32 2, ptr %arrayidx, align 4
+ %0 = load i64, ptr %A, align 8
+ ret i64 %0
+}
diff --git a/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll b/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll
new file mode 100644
index 00000000000000..2d80b2493a389b
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/FlipFlopBaseAddress.ll
@@ -0,0 +1,136 @@
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: | FileCheck %s
+
+; Check that dependence analysis correctly handles flip-flop of base addresses.
+; Bug 41488 - https://github.com/llvm/llvm-project/issues/41488
+
+; CHECK-LABEL: bug41488_test1
+; CHECK-NOT: da analyze - none!
+
+define float @bug41488_test1() {
+entry:
+ %g = alloca float, align 4
+ %h = alloca float, align 4
+ br label %for.body
+
+for.body:
+ %p = phi float* [ %g, %entry ], [ %q, %for.body ]
+ %q = phi float* [ %h, %entry ], [ %p, %for.body ]
+ %0 = load float, float* %p, align 4
+ store float undef, float* %q, align 4
+ %branch_cond = fcmp ugt float %0, 0.0
+ br i1 %branch_cond, label %for.cond.cleanup, label %for.body
+
+for.cond.cleanup:
+ ret float undef
+}
+
+; CHECK-LABEL: bug41488_test2
+; CHECK-NOT: da analyze - none!
+
+define void @bug41488_test2(i32 %n) {
+entry:
+ %g = alloca float, align 4
+ %h = alloca float, align 4
+ br label %for.body
+
+for.body:
+ %i = phi i32 [0, %entry ], [ %inc, %for.body ]
+ %p = phi float* [ %g, %entry ], [ %q, %for.body ]
+ %q = phi float* [ %h, %entry ], [ %p, %for.body ]
+ %0 = load float, float* %p, align 4
+ store float 0.0, float* %q, align 4
+ %inc = add nuw i32 %i, 1
+ %branch_cond = icmp ult i32 %i, %n
+ br i1 %branch_cond, label %for.body, label %for.cond.cleanup
+
+for.cond.cleanup:
+ ret void
+}
+
+; Bug 53942 - https://github.com/llvm/llvm-project/issues/53942
+; CHECK-LABEL: bug53942_foo
+; CHECK-NOT: da analyze - none!
+
+define void @bug53942_foo(i32 noundef %n, ptr noalias nocapture noundef writeonly %A, ptr noalias nocapture noundef %B) {
+entry:
+ %cmp8 = icmp sgt i32 %n, 1
+ br i1 %cmp8, label %for.body.preheader, label %for.cond.cleanup
+
+for.body.preheader: ; preds = %entry
+ %wide.trip.count = zext nneg i32 %n to i64
+ br label %for.body
+
+for.cond.cleanup: ; preds = %for.body, %entry
+ ret void
+
+for.body: ; preds = %for.body.preheader, %for.body
+ %indvars.iv = phi i64 [ 1, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+ %ptr1.011 = phi ptr [ %A, %for.body.preheader ], [ %ptr2.09, %for.body ]
+ %ptr2.09 = phi ptr [ %B, %for.body.preheader ], [ %ptr1.011, %for.body ]
+ %.pre = load double, ptr %B, align 8
+ %arrayidx2 = getelementptr inbounds double, ptr %ptr1.011, i64 %indvars.iv
+ store double %.pre, ptr %arrayidx2, align 8
+ %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+ %exitcond.not = icmp eq i64 %indvars.iv.next, %wide.trip.count
+ br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+}
+
+
+; Bug 53942 - https://github.com/llvm/llvm-project/issues/53942
+; CHECK-LABEL: bug53942_bar
+; CHECK-NOT: da analyze - none!
+
+define void @bug53942_bar(i32 noundef %n, ptr noalias noundef %A, ptr noalias noundef %B) {
+entry:
+ br label %for.cond
+
+for.cond: ; preds = %for.inc, %entry
+ %i.0 = phi i32 [ 1, %entry ], [ %inc, %for.inc ]
+ %cmp = icmp slt i32 %i.0, %n
+ br i1 %cmp, label %for.body, label %for.cond.cleanup
+
+for.cond.cleanup: ; preds = %for.cond
+ br label %for.end
+
+for.body: ; preds = %for.cond
+ %and = and i32 %i.0, 2
+ %tobool.not = icmp eq i32 %and, 0
+ br i1 %tobool.not, label %cond.false, label %cond.true
+
+cond.true: ; preds = %for.body
+ br label %cond.end
+
+cond.false: ; preds = %for.body
+ br label %cond.end
+
+cond.end: ; preds = %cond.false, %cond.true
+ %cond = phi ptr [ %A, %cond.true ], [ %B, %cond.false ]
+ %and1 = and i32 %i.0, 2
+ %tobool2.not = icmp eq i32 %and1, 0
+ br i1 %tobool2.not, label %cond.false4, label %cond.true3
+
+cond.true3: ; preds = %cond.end
+ br label %cond.end5
+
+cond.false4: ; preds = %cond.end
+ br label %cond.end5
+
+cond.end5: ; preds = %cond.false4, %cond.true3
+ %cond6 = phi ptr [ %B, %cond.true3 ], [ %A, %cond.false4 ]
+ %sub = add nsw i32 %i.0, -1
+ %idxprom = sext i32 %sub to i64
+ %arrayidx = getelementptr inbounds double, ptr %cond6, i64 %idxprom
+ %0 = load double, ptr %arrayidx, align 8
+ %idxprom7 = zext nneg i32 %i.0 to i64
+ %arrayidx8 = getelementptr inbounds double, ptr %cond, i64 %idxprom7
+ store double %0, ptr %arrayidx8, align 8
+ br label %for.inc
+
+for.inc: ; preds = %cond.end5
+ %inc = add nuw nsw i32 %i.0, 1
+ br label %for.cond
+
+for.end: ; preds = %for.cond.cleanup
+ ret void
+}
diff --git a/llvm/test/Analysis/DependenceAnalysis/PR31848.ll b/llvm/test/Analysis/DependenceAnalysis/PR31848.ll
new file mode 100644
index 00000000000000..02e7c54f9e941d
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/PR31848.ll
@@ -0,0 +1,67 @@
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: | FileCheck %s
+
+define void @foo(i32* nocapture %A, i32 %n) {
+entry:
+ %n.cmp = icmp sgt i32 %n, 0
+ br i1 %n.cmp, label %for.j.header, label %exit
+
+for.j.header:
+ %j= phi i32 [ %j.inc, %for.j.latch ], [ 0, %entry ]
+ br label %for.i.header
+
+for.i.header:
+ %i = phi i32 [ %i.inc, %for.i.latch ], [ 0, %for.j.header ]
+ br label %for.di.header
+
+for.di.header:
+ %di = phi i32 [ -1, %for.i.header ], [ %di.inc, %for.di.latch ]
+ %di.add = add nsw i32 %di, %i
+ br label %for.dj.header
+
+for.dj.header:
+ %dj = phi i32 [ -1, %for.di.header ], [ %dj.inc, %body ]
+ %dj.add = add nsw i32 %dj, %j
+ br label %while.x
+
+while.x:
+ %x = phi i32 [ %di.add, %for.dj.header ], [ %x.inc, %while.x ]
+ %x.cmp = icmp slt i32 %x, 0
+ %x.inc = add nsw i32 %x, %n
+ br i1 %x.cmp, label %while.x, label %while.y
+
+while.y:
+ %y = phi i32 [ %y.inc, %while.y ], [ %dj.add, %while.x ]
+ %y.cmp = icmp slt i32 %y, 0
+ %y.inc = add nsw i32 %y, %n
+ br i1 %y.cmp, label %while.y, label %body
+
+body:
+ %mul = mul nsw i32 %y, %n
+ %add = add nsw i32 %mul, %x
+ %idxprom = sext i32 %add to i64
+ %arrayidx = getelementptr inbounds i32, i32* %A, i64 %idxprom
+; CHECK: da analyze - output [* * * *]
+ store i32 7, i32* %arrayidx, align 4
+ %dj.inc = add nsw i32 %dj, 1
+ %dj.exitcond = icmp eq i32 %dj.inc, 2
+ br i1 %dj.exitcond, label %for.di.latch, label %for.dj.header
+
+for.di.latch:
+ %di.inc = add nsw i32 %di, 1
+ %di.exitcond = icmp eq i32 %di.inc, 2
+ br i1 %di.exitcond, label %for.i.latch, label %for.di.header
+
+for.i.latch:
+ %i.inc = add nuw nsw i32 %i, 1
+ %i.exitcond = icmp eq i32 %i.inc, %n
+ br i1 %i.exitcond, label %for.j.latch, label %for.i.header
+
+for.j.latch:
+ %j.inc = add nuw nsw i32 %j, 1
+ %j.exitcond = icmp eq i32 %j.inc, %n
+ br i1 %j.exitcond, label %exit, label %for.j.header
+
+exit:
+ ret void
+}
diff --git a/llvm/test/Analysis/DependenceAnalysis/PR51512.ll b/llvm/test/Analysis/DependenceAnalysis/PR51512.ll
new file mode 100644
index 00000000000000..52419adff8f8b5
--- /dev/null
+++ b/llvm/test/Analysis/DependenceAnalysis/PR51512.ll
@@ -0,0 +1,34 @@
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa
+
+; Check that the testcase does not crash the compiler.
+; https://github.com/llvm/llvm-project/issues/51512
+
+define void @func_1() {
+entry:
+ %l_83.i.i = alloca [2 x [5 x i32]], align 1
+ br label %for.cond857.preheader.i.i
+
+for.cond857.preheader.i.i: ; preds = %cleanup.cont1138.i.i, %entry
+ %l_89.08.i.i = phi i32 [ 0, %entry ], [ %add1140.i.i, %cleanup.cont1138.i.i ]
+ %0 = trunc i32 %l_89.08.i.i to i16
+ %1 = add i16 %0, 3
+ %arrayidx916.i.i = getelementptr inbounds [2 x [5 x i32]], [2 x [5 x i32]]* %l_83.i.i, i16 0, i16 %0, i16 %1
+ br label %for.body860.i.i
+
+for.body860.i.i: ; preds = %for.body860.i.i, %for.cond857.preheader.i.i
+ %l_74.07.i.i = phi i32 [ 0, %for.cond857.preheader.i.i ], [ %add964.i.i, %for.body860.i.i ]
+ store i32 undef, i32* %arrayidx916.i.i, align 1
+ %2 = trunc i32 %l_74.07.i.i to i16
+ %arrayidx962.i.i = getelementptr inbounds [2 x [5 x i32]], [2 x [5 x i32]]* %l_83.i.i, i16 0, i16 %2, i16 %1
+ store i32 0, i32* %arrayidx962.i.i, align 1
+ %add964.i.i = add nuw nsw i32 %l_74.07.i.i, 1
+ br i1 false, label %for.body860.i.i, label %cleanup.cont1138.i.i
+
+cleanup.cont1138.i.i: ; preds = %for.body860.i.i
+ %add1140.i.i = add nuw nsw i32 %l_89.08.i.i, 1
+ %cmp602.i.i = icmp eq i32 %l_89.08.i.i, 0
+ br i1 %cmp602.i.i, label %for.cond857.preheader.i.i, label %for.cond1480.i.i.preheader
+
+for.cond1480.i.i.preheader: ; preds = %cleanup.cont1138.i.i
+ unreachable
+}
|
Yeah, I think we want to review and submit them separately, not squash them all together, so multiple PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Array Dependence Analysis (DA) was built based on several assumptions (mostly coming from properties guaranteed by the Fortran language.) DA needs to conservatively answer "don't know" when the assumptions are not met. This patch series adds code to detect some of the assumptions.
The first patch [DA] disambiguate evolution of base addresses ensures that the base address is invariant in DA. The patch uses SCEV analysis. This fixes two bugs: #41488 and #53942
The second patch [DA] Dependence analysis does not handle array accesses of different sizes makes sure all memory accesses have the same byte width. This fixes bug #16183
The third patch [DA] add testcase adds a missing test for a bug that seems to be already fixed: #31196
The fourth patch [DA] use NSW arithmetic fixes #51512 where the constraints solver in DA was using wrap-around instead of NSW arithmetic.
The patch series was tested on arm64-linux.