[DA] Check for overflow in strong SIV test #166223
Conversation
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Alireza Torabian (1997alireza) ChangesFixing reverted PR #164704. Overflow is checked in the strong SIV test in DA on calculation of the product of Full diff: https://github.com/llvm/llvm-project/pull/166223.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 11d829492a10e..e45d1f79b3165 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -1587,6 +1587,15 @@ static const SCEV *minusSCEVNoSignedOverflow(const SCEV *A, const SCEV *B,
return nullptr;
}
+/// Returns \p A * \p B if it guaranteed not to signed wrap. Otherwise returns
+/// nullptr. \p A and \p B must have the same integer type.
+static const SCEV *mulSCEVNoSignedOverflow(const SCEV *A, const SCEV *B,
+ ScalarEvolution &SE) {
+ if (SE.willNotOverflow(Instruction::Mul, /*Signed=*/true, A, B))
+ return SE.getMulExpr(A, B);
+ return nullptr;
+}
+
/// Returns the absolute value of \p A. In the context of dependence analysis,
/// we need an absolute value in a mathematical sense. If \p A is the signed
/// minimum value, we cannot represent it unless extending the original type.
@@ -1686,7 +1695,11 @@ bool DependenceInfo::strongSIVtest(const SCEV *Coeff, const SCEV *SrcConst,
assert(0 < Level && Level <= CommonLevels && "level out of range");
Level--;
- const SCEV *Delta = SE->getMinusSCEV(SrcConst, DstConst);
+ const SCEV *Delta = minusSCEVNoSignedOverflow(SrcConst, DstConst, *SE);
+ if (!Delta) {
+ Result.Consistent = false;
+ return false;
+ }
LLVM_DEBUG(dbgs() << "\t Delta = " << *Delta);
LLVM_DEBUG(dbgs() << ", " << *Delta->getType() << "\n");
@@ -1702,7 +1715,9 @@ bool DependenceInfo::strongSIVtest(const SCEV *Coeff, const SCEV *SrcConst,
const SCEV *AbsCoeff = absSCEVNoSignedOverflow(Coeff, *SE);
if (!AbsDelta || !AbsCoeff)
return false;
- const SCEV *Product = SE->getMulExpr(UpperBound, AbsCoeff);
+ const SCEV *Product = mulSCEVNoSignedOverflow(UpperBound, AbsCoeff, *SE);
+ if (!Product)
+ return false;
return isKnownPredicate(CmpInst::ICMP_SGT, AbsDelta, Product);
}();
if (IsDeltaLarge) {
diff --git a/llvm/test/Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll b/llvm/test/Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll
index 4346507ba8f90..181a4494b036e 100644
--- a/llvm/test/Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll
@@ -210,7 +210,7 @@ define void @t3(i64 %n, i64 %m, i64 %lb, ptr %a) {
; CHECK-NEXT: Src: %2 = load i32, ptr %arrayidx6, align 4 --> Dst: %2 = load i32, ptr %arrayidx6, align 4
; CHECK-NEXT: da analyze - none!
; CHECK-NEXT: Src: %2 = load i32, ptr %arrayidx6, align 4 --> Dst: store i32 %2, ptr %arrayidx8, align 4
-; CHECK-NEXT: da analyze - consistent anti [1 -2]!
+; CHECK-NEXT: da analyze - anti [1 *]!
; CHECK-NEXT: Src: store i32 %2, ptr %arrayidx8, align 4 --> Dst: store i32 %2, ptr %arrayidx8, align 4
; CHECK-NEXT: da analyze - none!
;
diff --git a/llvm/test/Analysis/DependenceAnalysis/StrongSIV.ll b/llvm/test/Analysis/DependenceAnalysis/StrongSIV.ll
index 44bd9b7727910..160196284f415 100644
--- a/llvm/test/Analysis/DependenceAnalysis/StrongSIV.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/StrongSIV.ll
@@ -1,6 +1,8 @@
; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
-; RUN: | FileCheck %s
+; RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-ALL
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa -da-enable-dependence-test=strong-siv 2>&1 \
+; RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-STRONG-SIV
target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.6.0"
@@ -423,19 +425,33 @@ for.end: ; preds = %for.body
;; *B++ = A[i + 2*n];
define void @strong9(ptr %A, ptr %B, i64 %n) nounwind uwtable ssp {
-; CHECK-LABEL: 'strong9'
-; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
-; CHECK-NEXT: da analyze - none!
-; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
-; CHECK-NEXT: da analyze - none!
-; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
-; CHECK-NEXT: da analyze - confused!
-; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
-; CHECK-NEXT: da analyze - none!
-; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
-; CHECK-NEXT: da analyze - confused!
-; CHECK-NEXT: Src: store i32 %0, ptr %B.addr.02, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
-; CHECK-NEXT: da analyze - none!
+; CHECK-ALL-LABEL: 'strong9'
+; CHECK-ALL-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
+; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
+; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-ALL-NEXT: da analyze - confused!
+; CHECK-ALL-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
+; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-ALL-NEXT: da analyze - confused!
+; CHECK-ALL-NEXT: Src: store i32 %0, ptr %B.addr.02, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-ALL-NEXT: da analyze - none!
+;
+; CHECK-STRONG-SIV-LABEL: 'strong9'
+; CHECK-STRONG-SIV-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - none!
+; CHECK-STRONG-SIV-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - flow [*|<]!
+; CHECK-STRONG-SIV-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - confused!
+; CHECK-STRONG-SIV-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - none!
+; CHECK-STRONG-SIV-NEXT: Src: %0 = load i32, ptr %arrayidx2, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - confused!
+; CHECK-STRONG-SIV-NEXT: Src: store i32 %0, ptr %B.addr.02, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - none!
;
entry:
%cmp1 = icmp eq i64 %n, 0
@@ -512,3 +528,45 @@ for.body: ; preds = %entry, %for.body
for.end: ; preds = %for.body
ret void
}
+
+
+;; for (long unsigned i = 0; i < 9223372036854775806; i++)
+;; for (long unsigned j = 0; j < 2147483640; j++)
+;; if (i < 3000000000)
+;; A[i] = 0;
+;
+; FIXME: DependenceAnalysis fails to detect the dependency between A[i] and
+; itself, while Strong SIV has been able to prove it.
+define void @strong11(ptr %A) nounwind uwtable ssp {
+; CHECK-ALL-LABEL: 'strong11'
+; CHECK-ALL-NEXT: Src: store i32 0, ptr %arrayidx, align 4 --> Dst: store i32 0, ptr %arrayidx, align 4
+; CHECK-ALL-NEXT: da analyze - none!
+;
+; CHECK-STRONG-SIV-LABEL: 'strong11'
+; CHECK-STRONG-SIV-NEXT: Src: store i32 0, ptr %arrayidx, align 4 --> Dst: store i32 0, ptr %arrayidx, align 4
+; CHECK-STRONG-SIV-NEXT: da analyze - consistent output [0 S]!
+;
+entry:
+ br label %for.cond1.preheader
+
+for.cond1.preheader: ; preds = %entry, %for.cond.cleanup3
+ %i.017 = phi i64 [ 0, %entry ], [ %inc8, %for.cond.cleanup3 ]
+ %cmp5 = icmp samesign ult i64 %i.017, 3000000000
+ %arrayidx = getelementptr inbounds nuw i32, ptr %A, i64 %i.017
+ br i1 %cmp5, label %for.body4.us, label %for.cond.cleanup3
+
+for.body4.us: ; preds = %for.cond1.preheader, %for.body4.us
+ %j.016.us = phi i64 [ %inc.us, %for.body4.us ], [ 0, %for.cond1.preheader ]
+ store i32 0, ptr %arrayidx, align 4
+ %inc.us = add nuw nsw i64 %j.016.us, 1
+ %exitcond.not = icmp eq i64 %inc.us, 2147483640
+ br i1 %exitcond.not, label %for.cond.cleanup3, label %for.body4.us
+
+for.cond.cleanup: ; preds = %for.cond.cleanup3
+ ret void
+
+for.cond.cleanup3: ; preds = %for.body4.us, %for.cond1.preheader
+ %inc8 = add nuw nsw i64 %i.017, 1
+ %exitcond19.not = icmp eq i64 %inc8, 9223372036854775806
+ br i1 %exitcond19.not, label %for.cond.cleanup, label %for.cond1.preheader
+}
diff --git a/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll b/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll
index bf0fafcbfd6c9..4d6de1723404e 100644
--- a/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll
@@ -12,19 +12,24 @@
; A[2*i - 4] = 2;
; }
;
-; FIXME: DependenceAnalysis currently detects no dependency between the two
-; stores, but it does exist. For example, each store will access A[0] when i
-; is 1 and 2 respectively.
-; The root cause is that the product of the BTC and the coefficient
-; ((1LL << 62) - 1 and 2) overflows in a signed sense.
+; FIXME: DependenceAnalysis fails to detect the dependency between A[i] and
+; itself, while Strong SIV has been able to prove it.
define void @strongsiv_const_ovfl(ptr %A) {
-; CHECK-LABEL: 'strongsiv_const_ovfl'
-; CHECK-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
-; CHECK-NEXT: da analyze - none!
-; CHECK-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
-; CHECK-NEXT: da analyze - none!
-; CHECK-NEXT: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
-; CHECK-NEXT: da analyze - none!
+; CHECK-ALL-LABEL: 'strongsiv_const_ovfl'
+; CHECK-ALL-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
+; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
+; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
+; CHECK-ALL-NEXT: da analyze - none!
+;
+; CHECK-STRONG-SIV-LABEL: 'strongsiv_const_ovfl'
+; CHECK-STRONG-SIV-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
+; CHECK-STRONG-SIV-NEXT: da analyze - none!
+; CHECK-STRONG-SIV-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
+; CHECK-STRONG-SIV-NEXT: da analyze - consistent output [1]!
+; CHECK-STRONG-SIV-NEXT: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
+; CHECK-STRONG-SIV-NEXT: da analyze - none!
;
entry:
br label %loop.header
@@ -64,5 +69,4 @@ exit:
ret void
}
;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
-; CHECK-ALL: {{.*}}
-; CHECK-STRONG-SIV: {{.*}}
+; CHECK: {{.*}}
|
1997alireza
force-pushed
the
strong-siv-overflow-2
branch
from
8047eb8 to
74c5735
Compare
Rely on the product of `UpperBound` and `AbsCoeff` only if SCEV can prove that there is no overflow. Also the same about the result of the subtraction of `DstConst` from `SrcConst` to calculate `Delta`.
1997alireza
force-pushed
the
strong-siv-overflow-2
branch
from
5839c16 to
f372751
Compare
In these two issues we have overflow on calculation of the product |
But yes, proving the dependency is not what this patch provides, if that is what you mean. |
Yes, generally speaking we may not be able to prove dependency. I just removed the phrase from the comment. (I believe that will be used as your commit message when we merge). Just want it to be more accurate. Thanks for clarification. |
| const SCEV *Delta = SE->getMinusSCEV(SrcConst, DstConst); | ||
| const SCEV *Delta = minusSCEVNoSignedOverflow(SrcConst, DstConst, *SE); | ||
| if (!Delta) { | ||
| Result.Consistent = false; |
There was a problem hiding this comment.
Does Consistent need to be set to false?
There was a problem hiding this comment.
Yes, otherwise we will add the Consistent attribute to a few test cases that do not currently have this attribute. As you are also aware, the attribute Consistent seems to not be well defined. If it is true, we may need to try to remove it in other patches.
| ; FIXME: DependenceAnalysis fails to detect the dependency between A[i] and | ||
| ; itself, while Strong SIV has been able to prove it. |
There was a problem hiding this comment.
I think this comment also needs to be fixed, as Strong SIV doesn't prove the dependency.
There was a problem hiding this comment.
Strong SIV is proving the dependency after the fix by this patch. The output of Strong SIV is consistent output [0 S]!.
There was a problem hiding this comment.
I don't think DA proves the dependency. Returning false in strongSIVtest (same as other XXXtest functions) doesn't mean the presence of dependency; it just means that DA couldn't prove the absence of dependency.
There was a problem hiding this comment.
In general you are correct. If a test returns false it just mean that it couldn't prove anything. But in this case it is different. Sometime when StrongSIV is providing a distance or direction, it actually means that StrongSIV has been able to prove a dependency by providing their difference or direction. More specifically, whenever we have StrongSIVsuccesses++ in the StrongSIV test, it means that the test could successfully prove (returning false) or disprove a dependency (returning true).
This is something I had mentioned before on another discussion. When StrongSIV is successful to prove a dependency, DA does not need to check other tests anymore.
Anyway, meanwhile I think StrongSIV is actually capable to prove a dependency, I am fine to change the comment to something like this:
FIXME: DependenceAnalysis fails to detect the dependency between A[i] and itself, and Strong SIV is not the reason of this failure.
Let me know what you think or suggest. Thanks.
There was a problem hiding this comment.
In general you are correct. If a test returns false it just mean that it couldn't prove anything. But in this case it is different. Sometime when StrongSIV is providing a distance or direction, it actually means that StrongSIV has been able to prove a dependency by providing their difference or direction. More specifically, whenever we have
StrongSIVsuccesses++in the StrongSIV test, it means that the test could successfully prove (returning false) or disprove a dependency (returning true).This is something I had mentioned before on another discussion. When StrongSIV is successful to prove a dependency, DA does not need to check other tests anymore.
Hmm, I've reconsidered things a bit and now this looks reasonable to me. But...
FIXME: DependenceAnalysis fails to detect the dependency between A[i] and itself, and Strong SIV is not the reason of this failure.
I'd prefer this one.
| ; FIXME: DependenceAnalysis currently detects no dependency between the two | ||
| ; stores, but it does exist. For example, each store will access A[0] when i | ||
| ; is 1 and 2 respectively. | ||
| ; The root cause is that the product of the BTC and the coefficient | ||
| ; ((1LL << 62) - 1 and 2) overflows in a signed sense. |
There was a problem hiding this comment.
Why did you rewrite this comment?
There was a problem hiding this comment.
Because now the issue that has been mentioned in the comment regarding the overflow in the product of the BTC and the coefficient is resolved. By this patch, Strong SIV is generating the expected output consistent output [1]!. Now the problem is out of the Strong SIV scope that is leading the DA to generate an incorrect result.
There was a problem hiding this comment.
This test is not the same as @strongSIV11. It doesn't contain the access to A[i]. If you want to modify it, please follow the above pseudo code.
There was a problem hiding this comment.
I am not sure what you mean. Please clarify your comment on this change. No it is not same as @strongSIV11 and it does not contain an access to A[i]. The result provided by StrongSIV consistent output [1]! is correct and is regarding the store to A[2*i - 4] versus A[2*i - 2].
There was a problem hiding this comment.
No it is not same as
@strongSIV11and it does not contain an access toA[i]
But the comment is now saying FIXME: DependenceAnalysis fails to detect the dependency between A[i]....
There was a problem hiding this comment.
Thanks for noticing. Overlooked it. Now it is fixed.
There was a problem hiding this comment.
It doesn't seem to be fixed. Maybe forget to commit?
4 participants
Reland reverted PR #164704.
Overflow is checked in the strong SIV test in DA on calculation of the product of
UpperBoundandAbsCoeff, and also the subtraction ofDstConstfromSrcConst.This patch resolves the issues with the strong SIV test in both #159846 and #164246.