Skip to content

[DA] Check for overflow in strong SIV test #166223

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+108 −31
Open

[DA] Check for overflow in strong SIV test #166223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
59ce261
[DA] Adding a test case failure due to overflow in strong SIV test
1997alireza Oct 28, 2025
f372751
[DA] Check for overflow in strong SIV test
1997alireza Oct 22, 2025
a94c3ed
Merge branch 'main' into strong-siv-overflow-2
1997alireza Nov 4, 2025
c1ba53e
Merge branch 'main' into strong-siv-overflow-2
1997alireza Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions llvm/lib/Analysis/DependenceAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Consistent need to be set to false?

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}
LLVM_DEBUG(dbgs() << "\t Delta = " << *Delta);
LLVM_DEBUG(dbgs() << ", " << *Delta->getType() << "\n");

Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!
;
Expand Down
86 changes: 72 additions & 14 deletions llvm/test/Analysis/DependenceAnalysis/StrongSIV.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Comment on lines +538 to +539
Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment also needs to be fixed, as Strong SIV doesn't prove the dependency.

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong SIV is proving the dependency after the fix by this patch. The output of Strong SIV is consistent output [0 S]!.

Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
}
32 changes: 18 additions & 14 deletions llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines -15 to -19
Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you rewrite this comment?

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Contributor

@kasuga-fj kasuga-fj Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not same as @strongSIV11 and it does not contain an access to A[i]

But the comment is now saying FIXME: DependenceAnalysis fails to detect the dependency between A[i]....

Copy link
Contributor Author

@1997alireza 1997alireza Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing. Overlooked it. Now it is fixed.

; 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
Expand Down Expand Up @@ -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: {{.*}}
Loading