[DA] Fix signed overflow bugs in weak-crossing SIV test#188100
[DA] Fix signed overflow bugs in weak-crossing SIV test#188100
Conversation
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Ruoyu Qiu (cabbaken) ChangesThis patch refactors Key changes:
This patch fixes the overflow in Weak Crossing SIV test, but test case Full diff: https://github.com/llvm/llvm-project/pull/188100.diff 5 Files Affected:
diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 31bc32a4aa22b..f81f943c01312 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -1382,7 +1382,10 @@ bool DependenceInfo::weakCrossingSIVtest(const SCEV *Coeff,
++WeakCrossingSIVapplications;
assert(0 < Level && Level <= CommonLevels && "Level out of range");
Level--;
- const SCEV *Delta = SE->getMinusSCEV(DstConst, SrcConst);
+ const SCEV *Delta = minusSCEVNoSignedOverflow(DstConst, SrcConst, *SE);
+ if (!Delta)
+ return false;
+
LLVM_DEBUG(dbgs() << "\t Delta = " << *Delta << "\n");
if (Delta->isZero()) {
Result.DV[Level].Direction &= ~Dependence::DVEntry::LT;
@@ -1396,32 +1399,56 @@ bool DependenceInfo::weakCrossingSIVtest(const SCEV *Coeff,
return false;
}
const SCEVConstant *ConstCoeff = dyn_cast<SCEVConstant>(Coeff);
- if (!ConstCoeff)
+ const SCEVConstant *ConstDelta = dyn_cast<SCEVConstant>(Delta);
+ if (!ConstCoeff || !ConstDelta)
return false;
- if (SE->isKnownNegative(ConstCoeff)) {
- ConstCoeff = dyn_cast<SCEVConstant>(SE->getNegativeSCEV(ConstCoeff));
- assert(ConstCoeff &&
- "dynamic cast of negative of ConstCoeff should yield constant");
- Delta = SE->getNegativeSCEV(Delta);
+ OverflowSafeSignedAPInt SafeCoeff(ConstCoeff->getAPInt());
+ OverflowSafeSignedAPInt SafeDelta(ConstDelta->getAPInt());
+
+ if (SafeCoeff->isNegative()) {
+ SafeCoeff = -SafeCoeff;
+ SafeDelta = -SafeDelta;
}
- assert(SE->isKnownPositive(ConstCoeff) && "ConstCoeff should be positive");
- const SCEVConstant *ConstDelta = dyn_cast<SCEVConstant>(Delta);
- if (!ConstDelta)
+ if (!SafeCoeff || !SafeDelta)
return false;
+ assert(SafeCoeff->isStrictlyPositive() && "ConstCoeff should be positive");
+
// We're certain that ConstCoeff > 0; therefore,
// if Delta < 0, then no dependence.
LLVM_DEBUG(dbgs() << "\t Delta = " << *Delta << "\n");
LLVM_DEBUG(dbgs() << "\t ConstCoeff = " << *ConstCoeff << "\n");
- if (SE->isKnownNegative(Delta)) {
+ if (SafeDelta->isNegative()) {
// No dependence, Delta < 0
++WeakCrossingSIVindependence;
++WeakCrossingSIVsuccesses;
return true;
}
+ // check that Coeff divides Delta
+ APInt Distance, Remainder;
+ APInt::sdivrem(*SafeDelta, *SafeCoeff, Distance, Remainder);
+ LLVM_DEBUG(dbgs() << "\t Remainder = " << Remainder << "\n");
+ if (Remainder != 0) {
+ // Coeff doesn't divide Delta, no dependence
+ ++WeakCrossingSIVindependence;
+ ++WeakCrossingSIVsuccesses;
+ return true;
+ }
+ LLVM_DEBUG(dbgs() << "\t Distance = " << Distance << "\n");
+
+ // if 2*Coeff doesn't divide Delta, then the equal direction isn't possible
+ APInt Two = APInt(Distance.getBitWidth(), 2, true);
+ Remainder = Distance.srem(Two);
+ LLVM_DEBUG(dbgs() << "\t Remainder = " << Remainder << "\n");
+ if (Remainder != 0) {
+ // Equal direction isn't possible
+ Result.DV[Level].Direction &= ~Dependence::DVEntry::EQ;
+ ++WeakCrossingSIVsuccesses;
+ }
+
// We're certain that Delta > 0 and ConstCoeff > 0.
// Check Delta/(2*ConstCoeff) against upper loop bound
if (const SCEV *UpperBound =
@@ -1431,8 +1458,11 @@ bool DependenceInfo::weakCrossingSIVtest(const SCEV *Coeff,
const SCEV *ML =
SE->getMulExpr(SE->getMulExpr(ConstCoeff, UpperBound), ConstantTwo);
LLVM_DEBUG(dbgs() << "\t ML = " << *ML << "\n");
- if (SE->isKnownPredicate(CmpInst::ICMP_SGT, Delta, ML)) {
- // Delta too big, no dependence
+ ConstantRange DeltaRange = SE->getSignedRange(Delta);
+ ConstantRange MLRange = SE->getSignedRange(ML);
+ LLVM_DEBUG(dbgs() << "\t DeltaRange = " << DeltaRange << "\n");
+ LLVM_DEBUG(dbgs() << "\t MLRange = " << MLRange << "\n");
+ if (DeltaRange.getSignedMin().sgt(MLRange.getSignedMax())) {
++WeakCrossingSIVindependence;
++WeakCrossingSIVsuccesses;
return true;
@@ -1450,31 +1480,6 @@ bool DependenceInfo::weakCrossingSIVtest(const SCEV *Coeff,
return false;
}
}
-
- // check that Coeff divides Delta
- APInt APDelta = ConstDelta->getAPInt();
- APInt APCoeff = ConstCoeff->getAPInt();
- APInt Distance = APDelta; // these need to be initialzed
- APInt Remainder = APDelta;
- APInt::sdivrem(APDelta, APCoeff, Distance, Remainder);
- LLVM_DEBUG(dbgs() << "\t Remainder = " << Remainder << "\n");
- if (Remainder != 0) {
- // Coeff doesn't divide Delta, no dependence
- ++WeakCrossingSIVindependence;
- ++WeakCrossingSIVsuccesses;
- return true;
- }
- LLVM_DEBUG(dbgs() << "\t Distance = " << Distance << "\n");
-
- // if 2*Coeff doesn't divide Delta, then the equal direction isn't possible
- APInt Two = APInt(Distance.getBitWidth(), 2, true);
- Remainder = Distance.srem(Two);
- LLVM_DEBUG(dbgs() << "\t Remainder = " << Remainder << "\n");
- if (Remainder != 0) {
- // Equal direction isn't possible
- Result.DV[Level].Direction &= ~Dependence::DVEntry::EQ;
- ++WeakCrossingSIVsuccesses;
- }
return false;
}
diff --git a/llvm/test/Analysis/DependenceAnalysis/WeakCrossingSIV.ll b/llvm/test/Analysis/DependenceAnalysis/WeakCrossingSIV.ll
index 6242763a525cb..36b040dbabbed 100644
--- a/llvm/test/Analysis/DependenceAnalysis/WeakCrossingSIV.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/WeakCrossingSIV.ll
@@ -68,7 +68,7 @@ define void @weakcrossing1(ptr %A, ptr %B, i64 %n) nounwind uwtable ssp {
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
; CHECK-NEXT: da analyze - output [*]!
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx2, align 4
-; CHECK-NEXT: da analyze - flow [<>]!
+; CHECK-NEXT: da analyze - flow [*|<]!
; 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
diff --git a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-delta-signed-min.ll b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-delta-signed-min.ll
index d089b8fd5dd6b..49abaf090386b 100644
--- a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-delta-signed-min.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-delta-signed-min.ll
@@ -12,14 +12,14 @@
; }
; }
;
-; FIXME: There is a dependency between the two stores in all directions.
+; There is a dependency between the two stores in all directions.
;
define void @weak_crossing_siv_delta_signed_min(ptr %A) {
; CHECK-ALL-LABEL: 'weak_crossing_siv_delta_signed_min'
; CHECK-ALL-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.0, align 1
; CHECK-ALL-NEXT: da analyze - none!
; CHECK-ALL-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.1, align 1
-; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: da analyze - output [*|<]!
; CHECK-ALL-NEXT: Src: store i8 1, ptr %gep.1, align 1 --> Dst: store i8 1, ptr %gep.1, align 1
; CHECK-ALL-NEXT: da analyze - none!
;
@@ -27,7 +27,7 @@ define void @weak_crossing_siv_delta_signed_min(ptr %A) {
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.0, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.1, align 1
-; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - none!
+; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*|<]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 1, ptr %gep.1, align 1 --> Dst: store i8 1, ptr %gep.1, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
;
diff --git a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-large-btc.ll b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-large-btc.ll
index 406dfaf96cfb7..d5ba08c9e8327 100644
--- a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-large-btc.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-large-btc.ll
@@ -13,7 +13,7 @@
; A[i1] = 0;
; }
;
-; FIXME: Both `A[i0] = 0` and `A[i1] = 0` must be executed, so there is a
+; Both `A[i0] = 0` and `A[i1] = 0` must be executed, so there is a
; dependency between them.
;
define void @weak_crossing_siv_large_btc(ptr %A) {
@@ -21,7 +21,7 @@ define void @weak_crossing_siv_large_btc(ptr %A) {
; CHECK-ALL-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.0, align 1
; CHECK-ALL-NEXT: da analyze - none!
; CHECK-ALL-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.1, align 1
-; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: da analyze - output [*|<]!
; CHECK-ALL-NEXT: Src: store i8 0, ptr %gep.1, align 1 --> Dst: store i8 0, ptr %gep.1, align 1
; CHECK-ALL-NEXT: da analyze - none!
;
@@ -29,7 +29,7 @@ define void @weak_crossing_siv_large_btc(ptr %A) {
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.0, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %gep.0, align 1 --> Dst: store i8 0, ptr %gep.1, align 1
-; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - none!
+; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*|<]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %gep.1, align 1 --> Dst: store i8 0, ptr %gep.1, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
;
diff --git a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-overflow.ll b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-overflow.ll
index acc1c0a69eb0f..9f9fe11816821 100644
--- a/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-overflow.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/weak-crossing-siv-overflow.ll
@@ -11,8 +11,9 @@
; A[3*i - 2] = 1;
; }
;
-; FIXME: DependenceAnalysis currently detects no dependency between
-; `A[-3*i + INT64_MAX]` and `A[3*i - 2]`, but it does exist. For example,
+;
+; There is a dependency between `A[-3*i + INT64_MAX]` and `A[3*i - 2]`,
+; for example,
;
; memory access | i == 1 | i == max_i
; ---------------------|------------------|------------------
@@ -27,7 +28,7 @@ define void @weakcorssing_delta_ovfl(ptr %A) {
; CHECK-ALL-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 0, ptr %idx.0, align 1
; CHECK-ALL-NEXT: da analyze - none!
; CHECK-ALL-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
-; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: da analyze - output [*|<]!
; CHECK-ALL-NEXT: Src: store i8 1, ptr %idx.1, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
; CHECK-ALL-NEXT: da analyze - none!
;
@@ -35,7 +36,7 @@ define void @weakcorssing_delta_ovfl(ptr %A) {
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 0, ptr %idx.0, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
-; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - none!
+; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*|<]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 1, ptr %idx.1, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
;
@@ -89,7 +90,7 @@ define void @weakcorssing_prod_ovfl(ptr %A) {
; CHECK-ALL-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 0, ptr %idx.0, align 1
; CHECK-ALL-NEXT: da analyze - none!
; CHECK-ALL-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
-; CHECK-ALL-NEXT: da analyze - none!
+; CHECK-ALL-NEXT: da analyze - output [*|<]!
; CHECK-ALL-NEXT: Src: store i8 1, ptr %idx.1, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
; CHECK-ALL-NEXT: da analyze - none!
;
@@ -97,7 +98,7 @@ define void @weakcorssing_prod_ovfl(ptr %A) {
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 0, ptr %idx.0, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 0, ptr %idx.0, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
-; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - none!
+; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*|<]!
; CHECK-WEAK-CROSSING-SIV-NEXT: Src: store i8 1, ptr %idx.1, align 1 --> Dst: store i8 1, ptr %idx.1, align 1
; CHECK-WEAK-CROSSING-SIV-NEXT: da analyze - output [*]!
;
|
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
This patch refactors `DependenceInfo::weakCrossingSIVtest` to fix the analysis against signed overflow during distance and loop bound calculations. Key changes: 1. Replaced `SE->getMinusSCEV` with `minusSCEVNoSignedOverflow`. 2. Adopted `OverflowSafeSignedAPInt` for `ConstCoeff` and `ConstDelta` to guarantee safe negation and manipulation. 3. Reordered the logic to check if `Coeff` exactly divides `Delta` (and `2*Coeff`) prior to checking loop bounds. This patch fixes the overflow in Weak Crossing SIV test, but test case `weakcrossing1` in WeakCrossingSIV.ll falled back from `[<>]` to `[*|<]` Signed-off-by: Ruoyu Qiu <cabbaken@outlook.com>
cabbaken
force-pushed
the
RefactorWeakCrossingSIVtest
branch
from
bba5ff6 to
cc80b33
Compare
|
Drive-in comment: refactoring shouldn't change the behavior of the code. Your changes appear to be bug fixes rather than refactoring. |
cabbaken
changed the title
kasuga-fj
left a comment
kasuga-fj
left a comment
There was a problem hiding this comment.
Could you please separate the PR? Even though all of the changes address overflow issues, it's a bit difficult to review because some of them are independent problems.
Of course, I'm working on it. |
| const SCEV *Delta = minusSCEVNoSignedOverflow(DstConst, SrcConst, *SE); | ||
| if (!Delta) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
@sjoerdmeijer @nikic @Meinersbur
At least this part has overlap with another patch that I have already posted.
https://github.com/llvm/llvm-project/pull/185046/changes
Also as @kasuga-fj is aware I have been working on weakCrossing test.
I am wondering how things are organized in the community so we do not work on the same thing and respect some boundary? I previously tried to coordinate my work with @kasuga-fj but didn't get a response. (Can find the comment if needed).
There was a problem hiding this comment.
so we do not work on the same thing
To be clear, my concern is about the case of having two patches that implement the same thing.
There was a problem hiding this comment.
Ah, sorry, that PR completely slipped my mind. I think it's not uncommon for two (or more) duplicate PRs to be submitted. In such cases, I believe it's usually standard practice to close one of them.
I previously tried to coordinate my work with @kasuga-fj but didn't get a response.
I remember you mentioned somewhere that you would take a look at the Weak Crossing SIV test. But that doesn’t mean you're preventing others from working on that part, does it.
|
@cabbaken could you please add me to DA patches as reviewer? I am actively working on it. |
|
@cabbaken My apologies if my comments were unexpected. I just felt very frustrated to see I am totally unaware of some ongoing work in DA that is related to the work that I am doing. This has happened before too. |
I learned about DA the issues with DA from this post. I apologise for not realising that you are working on DA too. I will add you and other relevant people as reviewers in the next few PRs. |
|
@amehsan I won't make any modifications to this PR or create any other PRs that modify this function. Please feel free to continue your work. |
I believe you don't need to stop your work if you would like to continue. In particular, regarding the Weak Crossing SIV test, it's already quite clear what needs to be fixed to make it sound. In such cases, I think the PR that is submitted first should take priority. Of course you are free to stop your work if you don't want to continue, but I don't think it's necessary. |
OK, I'll carry on working on this. Since part of my PR overlaps with @amehsan's, I will still create the related PR (because I need them to ensure the integrity of my work), you can choose whichever one you prefer. |
Nothing wrong on your part. It was my misinterpretation. |
My concern was about that patch that I have already posted. So feel free to continue your work. I tried to clarify in the subsequent comment. Sorry if it was not clear enough. |
4 participants
This patch fixs signed overflowing during distance and loop bound calculations
DependenceInfo::weakCrossingSIVtest.Key changes:
SE->getMinusSCEVwithminusSCEVNoSignedOverflow.OverflowSafeSignedAPIntforConstCoeffandConstDeltato guarantee safe negation and manipulation.Coeffexactly dividesDelta(and2*Coeff) prior to checking loop bounds.This patch fixes the overflow in Weak Crossing SIV test, but test case
weakcrossing1in WeakCrossingSIV.ll falled back from[<>]to[*|<]