-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[DA] Set Distance to zero when Direction is EQ #147966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-llvm-analysis Author: Ryotaro Kasuga (kasuga-fj) ChangesA Dependence object has two entries: Distance and Direction. The former represents the distance of the dependence, while the latter characterizes the distance by whether the value of it is positive, negative, or zero (especially, zero is represented by EQ in DA). So it is expected that the Distance equals zero iff the Direction is EQ. However, this condition was not satisfied in some cases. Full diff: https://github.com/llvm/llvm-project/pull/147966.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 5b85060f9caa1..df5fd0e938707 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -187,6 +187,18 @@ static void dumpExampleDependence(raw_ostream &OS, DependenceInfo *DA,
OS << " da analyze - ";
if (auto D = DA->depends(&*SrcI, &*DstI,
/*UnderRuntimeAssumptions=*/true)) {
+
+ // Verify that the distance begin zero is equivalent to the
+ // direction being EQ.
+ for (unsigned Level = 1; Level <= D->getLevels(); Level++) {
+ const SCEV *Distance = D->getDistance(Level);
+ bool IsDistanceZero = Distance && Distance->isZero();
+ bool IsDirectionEQ =
+ D->getDirection(Level) == Dependence::DVEntry::EQ;
+ assert(IsDistanceZero == IsDirectionEQ &&
+ "Inconsistent distance and direction.");
+ }
+
// Normalize negative direction vectors if required by clients.
if (NormalizeResults && D->normalize(&SE))
OS << "normalized - ";
@@ -3991,6 +4003,23 @@ DependenceInfo::depends(Instruction *Src, Instruction *Dst,
if (CompleteLoops[II])
Result.DV[II - 1].Scalar = false;
+ // Set the distance to zero if the direction is EQ.
+ // TODO: Ideally, the distance should be set to 0 immediately simultaneously
+ // with the corresponding direction being set to EQ.
+ for (unsigned II = 1; II <= Result.getLevels(); ++II) {
+ if (Result.getDirection(II) == Dependence::DVEntry::EQ)
+ Result.DV[II - 1].Distance = SE->getZero(SrcSCEV->getType());
+
+ LLVM_DEBUG({
+ // Check that the converse (i.e., if the distance is zero, then the
+ // direction is EQ) holds.
+ const SCEV *Distance = Result.getDistance(II);
+ if (Distance && Distance->isZero())
+ assert(Result.getDirection(II) == Dependence::DVEntry::EQ &&
+ "Distance is zero, but direction is not EQ");
+ });
+ }
+
if (PossiblyLoopIndependent) {
// Make sure the LoopIndependent flag is set correctly.
// All directions must include equal, otherwise no
diff --git a/llvm/test/Analysis/DependenceAnalysis/Banerjee.ll b/llvm/test/Analysis/DependenceAnalysis/Banerjee.ll
index 6768e9067dca3..d3301520fd107 100644
--- a/llvm/test/Analysis/DependenceAnalysis/Banerjee.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/Banerjee.ll
@@ -802,7 +802,7 @@ define void @banerjee9(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; CHECK-NEXT: da analyze - output [* *]!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
-; CHECK-NEXT: da analyze - flow [<= =|<]!
+; CHECK-NEXT: da analyze - flow [<= 0|<]!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; CHECK-NEXT: da analyze - confused!
; CHECK-NEXT: Src: %1 = load i64, ptr %arrayidx7, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
@@ -816,7 +816,7 @@ define void @banerjee9(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; NORMALIZE-NEXT: da analyze - output [* *]!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
-; NORMALIZE-NEXT: da analyze - flow [<= =|<]!
+; NORMALIZE-NEXT: da analyze - flow [<= 0|<]!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; NORMALIZE-NEXT: da analyze - confused!
; NORMALIZE-NEXT: Src: %1 = load i64, ptr %arrayidx7, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
@@ -830,7 +830,7 @@ define void @banerjee9(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; DELIN-NEXT: da analyze - output [* *]!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
-; DELIN-NEXT: da analyze - flow [<= =|<]!
+; DELIN-NEXT: da analyze - flow [<= 0|<]!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; DELIN-NEXT: da analyze - confused!
; DELIN-NEXT: Src: %1 = load i64, ptr %arrayidx7, align 8 --> Dst: %1 = load i64, ptr %arrayidx7, align 8
@@ -888,7 +888,7 @@ define void @banerjee10(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
-; CHECK-NEXT: da analyze - flow [<> =]!
+; CHECK-NEXT: da analyze - flow [<> 0]!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; CHECK-NEXT: da analyze - confused!
; CHECK-NEXT: Src: %1 = load i64, ptr %arrayidx6, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
@@ -902,7 +902,7 @@ define void @banerjee10(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; NORMALIZE-NEXT: da analyze - none!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
-; NORMALIZE-NEXT: da analyze - flow [<> =]!
+; NORMALIZE-NEXT: da analyze - flow [<> 0]!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; NORMALIZE-NEXT: da analyze - confused!
; NORMALIZE-NEXT: Src: %1 = load i64, ptr %arrayidx6, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
@@ -916,7 +916,7 @@ define void @banerjee10(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; DELIN-NEXT: da analyze - none!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
-; DELIN-NEXT: da analyze - flow [<> =]!
+; DELIN-NEXT: da analyze - flow [<> 0]!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %1, ptr %B.addr.11, align 8
; DELIN-NEXT: da analyze - confused!
; DELIN-NEXT: Src: %1 = load i64, ptr %arrayidx6, align 8 --> Dst: %1 = load i64, ptr %arrayidx6, align 8
@@ -1058,7 +1058,7 @@ define void @banerjee12(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
-; CHECK-NEXT: da analyze - flow [= <>]!
+; CHECK-NEXT: da analyze - flow [0 <>]!
; CHECK-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %0, ptr %B.addr.11, align 8
; CHECK-NEXT: da analyze - confused!
; CHECK-NEXT: Src: %0 = load i64, ptr %arrayidx6, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
@@ -1072,7 +1072,7 @@ define void @banerjee12(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; NORMALIZE-NEXT: da analyze - none!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
-; NORMALIZE-NEXT: da analyze - flow [= <>]!
+; NORMALIZE-NEXT: da analyze - flow [0 <>]!
; NORMALIZE-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %0, ptr %B.addr.11, align 8
; NORMALIZE-NEXT: da analyze - confused!
; NORMALIZE-NEXT: Src: %0 = load i64, ptr %arrayidx6, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
@@ -1086,7 +1086,7 @@ define void @banerjee12(ptr %A, ptr %B, i64 %m, i64 %n) nounwind uwtable ssp {
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 0, ptr %arrayidx, align 8
; DELIN-NEXT: da analyze - none!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
-; DELIN-NEXT: da analyze - flow [= <>]!
+; DELIN-NEXT: da analyze - flow [0 <>]!
; DELIN-NEXT: Src: store i64 0, ptr %arrayidx, align 8 --> Dst: store i64 %0, ptr %B.addr.11, align 8
; DELIN-NEXT: da analyze - confused!
; DELIN-NEXT: Src: %0 = load i64, ptr %arrayidx6, align 8 --> Dst: %0 = load i64, ptr %arrayidx6, align 8
diff --git a/llvm/test/Analysis/DependenceAnalysis/Coupled.ll b/llvm/test/Analysis/DependenceAnalysis/Coupled.ll
index ff9f393f88152..06bfc5d2e8573 100644
--- a/llvm/test/Analysis/DependenceAnalysis/Coupled.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/Coupled.ll
@@ -285,7 +285,7 @@ define void @couple6(ptr %A, ptr %B, i32 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx1, align 4 --> Dst: store i32 %conv, ptr %arrayidx1, align 4
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx1, align 4 --> Dst: %0 = load i32, ptr %arrayidx3, align 4
-; CHECK-NEXT: da analyze - flow [=|<]!
+; CHECK-NEXT: da analyze - flow [0|<]!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx1, align 4 --> Dst: store i32 %0, ptr %B.addr.01, align 4
; CHECK-NEXT: da analyze - confused!
; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx3, align 4 --> Dst: %0 = load i32, ptr %arrayidx3, align 4
@@ -503,7 +503,7 @@ define void @couple11(ptr %A, ptr %B, i32 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx2, align 4 --> Dst: store i32 %conv, ptr %arrayidx2, align 4
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx2, align 4 --> Dst: %0 = load i32, ptr %arrayidx4, align 4
-; CHECK-NEXT: da analyze - flow [=|<] splitable!
+; CHECK-NEXT: da analyze - flow [0|<] splitable!
; CHECK-NEXT: da analyze - split level = 1, iteration = 9!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx2, align 4 --> Dst: store i32 %0, ptr %B.addr.01, align 4
; CHECK-NEXT: da analyze - confused!
@@ -636,7 +636,7 @@ define void @couple14(ptr %A, ptr %B, i32 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx3, align 4 --> Dst: store i32 %conv, ptr %arrayidx3, align 4
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx3, align 4 --> Dst: %0 = load i32, ptr %arrayidx6, align 4
-; CHECK-NEXT: da analyze - flow [=|<]!
+; CHECK-NEXT: da analyze - flow [0|<]!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx3, align 4 --> Dst: store i32 %0, ptr %B.addr.01, align 4
; CHECK-NEXT: da analyze - confused!
; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx6, align 4 --> Dst: %0 = load i32, ptr %arrayidx6, align 4
diff --git a/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll b/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
index f0cd2fd4cd930..e5d5d21e365a1 100644
--- a/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
@@ -18,7 +18,7 @@ define void @i32_subscript(ptr %a, ptr %b) {
; CHECK-NEXT: Src: %0 = load i32, ptr %a.addr, align 4 --> Dst: %0 = load i32, ptr %a.addr, align 4
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: %0 = load i32, ptr %a.addr, align 4 --> Dst: store i32 %1, ptr %a.addr.2, align 4
-; CHECK-NEXT: da analyze - anti [=|<]!
+; CHECK-NEXT: da analyze - anti [0|<]!
; CHECK-NEXT: Src: store i32 %1, ptr %a.addr.2, align 4 --> Dst: store i32 %1, ptr %a.addr.2, align 4
; CHECK-NEXT: da analyze - none!
;
|
| // TODO: Ideally, the distance should be set to 0 immediately simultaneously | ||
| // with the corresponding direction being set to EQ. | ||
| for (unsigned II = 1; II <= Result.getLevels(); ++II) { | ||
| if (Result.getDirection(II) == Dependence::DVEntry::EQ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it only that Distance is nullptr, or are there cases where it is non-zero but direction is still EQ?
In the former case, could you set the direction only when Distance is nullptr and assert if not SCEV::isZero?
In the latter case, the would be something quite wrong. Why prioritize the Direction analysis result over the Distance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former. I noticed it when I changed the code a bit and saw that some of the dumped results in the tests changed from = to 0. Added assert for the latter case.
| if (auto D = DA->depends(&*SrcI, &*DstI, | ||
| /*UnderRuntimeAssumptions=*/true)) { | ||
|
|
||
| // Verify that the distance begin zero is equivalent to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/begin/being?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks
Co-authored-by: Michael Kruse <[email protected]>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Meinersbur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks for the reviews! |
A Dependence object has two entries: Distance and Direction. The former represents the distance of the dependence, while the latter characterizes the distance by whether the value of it is positive, negative, or zero (especially, zero is represented by EQ in DA). So it is expected that the Distance equals zero iff the Direction is EQ. However, this condition was not satisfied in some cases.
This patch adds a logic to set the Distance to zero if the Direction is EQ. Although it is ideal that the Distance is updated to zero simultaneously when the Direction is set to EQ, achieving it would require changing the entire code in DA.