[DA] Check for overflow in strong SIV test #164704
Merged
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
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Alireza Torabian (1997alireza) ChangesBy doubling the size of the expressions in the strong SIV test, no overflow occurs in the multiplication This patch resolves the issue #159846, and fixes the strong SIV test created in the issue #164246 by proving the dependency. While this patch resolves the overflow bug in the strong SIV test, it also prevents the test from performing and disproving the dependency in some cases. The reason behind this is due to adding Full diff: https://github.com/llvm/llvm-project/pull/164704.diff 1 Files Affected:
diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index 853bd66c8a7f8..af900e2a92998 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -1668,17 +1668,35 @@ bool DependenceInfo::strongSIVtest(const SCEV *Coeff, const SCEV *SrcConst,
LLVM_DEBUG(dbgs() << "\t Delta = " << *Delta);
LLVM_DEBUG(dbgs() << ", " << *Delta->getType() << "\n");
+ TypeSize CoeffSize =
+ Coeff->getType()->getScalarType()->getPrimitiveSizeInBits();
+ TypeSize SrcSize =
+ SrcConst->getType()->getScalarType()->getPrimitiveSizeInBits();
+ TypeSize DstSize =
+ DstConst->getType()->getScalarType()->getPrimitiveSizeInBits();
+ TypeSize WideSize = std::max(CoeffSize, std::max(SrcSize, DstSize)) * 2;
+ LLVMContext &Context = CurDstLoop->getHeader()->getParent()->getContext();
+ Type *WideTy = IntegerType::get(Context, WideSize);
+ const SCEV *WideSrcC = SE->getSignExtendExpr(SrcConst, WideTy);
+ const SCEV *WideDstC = SE->getSignExtendExpr(DstConst, WideTy);
+ const SCEV *WideDelta = SE->getMinusSCEV(WideSrcC, WideDstC);
+ const SCEV *WideCoeff = SE->getSignExtendExpr(Coeff, WideTy);
+
// check that |Delta| < iteration count
- if (const SCEV *UpperBound =
- collectUpperBound(CurSrcLoop, Delta->getType())) {
- LLVM_DEBUG(dbgs() << "\t UpperBound = " << *UpperBound);
- LLVM_DEBUG(dbgs() << ", " << *UpperBound->getType() << "\n");
- const SCEV *AbsDelta =
- SE->isKnownNonNegative(Delta) ? Delta : SE->getNegativeSCEV(Delta);
- const SCEV *AbsCoeff =
- SE->isKnownNonNegative(Coeff) ? Coeff : SE->getNegativeSCEV(Coeff);
- const SCEV *Product = SE->getMulExpr(UpperBound, AbsCoeff);
- if (isKnownPredicate(CmpInst::ICMP_SGT, AbsDelta, Product)) {
+ if (const SCEV *WideUpperBound =
+ collectUpperBound(CurSrcLoop, WideDelta->getType())) {
+ LLVM_DEBUG(dbgs() << "\t WideUpperBound = " << *WideUpperBound);
+ LLVM_DEBUG(dbgs() << ", " << *WideUpperBound->getType() << "\n");
+
+ // FIXME: Use SCEV getAbsExpr function to compute the abstract values
+ const SCEV *WideAbsDelta = SE->isKnownNonNegative(WideDelta)
+ ? WideDelta
+ : SE->getNegativeSCEV(WideDelta);
+ const SCEV *WideAbsCoeff = SE->isKnownNonNegative(WideCoeff)
+ ? WideCoeff
+ : SE->getNegativeSCEV(WideCoeff);
+ const SCEV *WideProduct = SE->getMulExpr(WideUpperBound, WideAbsCoeff);
+ if (isKnownPredicate(CmpInst::ICMP_SGT, WideAbsDelta, WideProduct)) {
// Distance greater than trip count - no dependence
++StrongSIVindependence;
++StrongSIVsuccesses;
|
|
@kasuga-fj This patch addresses the issue with the strong SIV test you referenced in #164246. While strong SIV produces the correct result While we should resolve the bug in RDIV, I believe it may not be necessary for DA to perform the additional tests when strong SIV has already proved the dependency. Specifically, I’m referring to line 2717. Even if |
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
1f54190 to
271f32a
Compare
|
The change itself doesn't seem to be problematic, but as I mentioned in #159846 (comment), I'm now skeptical of the approach that uses wider integer types. For the cases I provided, where overflows are triggered by large constant values, I don't think they are practical enough to justify the added complexity of using wider integer types. While it's certainly beneficial to provide more accurate results in such cases, I believe it's sufficient to simply return an unknown dependency. |
What is the concern here, is it compile time or something else? large constant values are very rare in practice, so I don't think that should prevent us from merging this PR. While in principle I agree that the primary concern is correctness, and it is ok to give up on accuracy for correctness, if there is an approach that can give us more accurate results while preserving correctness, it should be preferrable. (EDIT: I haven't checked the code, but I expect for the cases where arithmetic operations on the constants do not result in overflow, widening the type should not have any impact on the complexity of the operations. Zero bits on the left side shouldn't matter). |
amehsan
reviewed
Oct 23, 2025
|
I meant the complexity of the code, not computational complexity. Since DA already has a lot of issues, I'd prefer to keep the code as simple as possible. In this specific case I think the simplest solution is to detect (perhaps potentially) overflows during the calculations and bail out if one is detected. If simply widening the integer type improves accuracy in many cases compared to bailing out when an overflow occurs, that would be great. However, looking at the tasks listed in #159846 (comment), I don't think that's the case. As you said we may be able to prove in some way that In conclusion, at the moment, I don't think widening the integer type justifies the complexity increase in the code. |
kasuga-fj
reviewed
Oct 23, 2025
| LLVM_DEBUG(dbgs() << "\t WideUpperBound = " << *WideUpperBound); | ||
| LLVM_DEBUG(dbgs() << ", " << *WideUpperBound->getType() << "\n"); | ||
|
|
||
| // FIXME: Use SCEV getAbsExpr function to compute the abstract values |
There was a problem hiding this comment.
JFYI: Abs means absolute value.
There was a problem hiding this comment.
Thanks, this is obviously just a mistake when writing the comment
That example is a separate issue. That is not related to this patch. and that case is not something that we want care about its accuracy.
The issue is not how many hanadwritten testcases can be improved. The issue is how often this can help in the real and unseen code. In an arithmetic operation where one of the involved expressions is symbolic, it is very difficult to prove the result won't overflow. Simply because often times we don't have any bound on the value of the symbolic expression. This is why I proposed widening in the first place. For symbolic intermediate computations in DA, without widening we have to give up on many real-world cases very quickly. @1997alireza can post an example to clarify this. The complexity increase is very small here. Once we miss a performance opportunity it is not easy to detect it. Once detected, it takes a lot of effort to find the root cause, come up with the idea and implement it. So if you consider longer term, I expect significant benefit from this patch. |
|
I'm okay if you have some real world examples (but I'm not entirely sure why wider type is useful even though we don't know the bounds of symbolic values). |
Consider this code: if but if we extend both values to 128 bit and multiply them, the result will definitely not overflow 128 bits. So in the next step Assuming |
|
But isn't that isKnownPredicate always going to fail in the case where the multiply may overflow? That means that the value is potentially larger than the original type, and AbsDelta fits within that type. Maybe I'm misunderstanding something here. By the way, it should be possible to write a test for this using |
if we exnted the type then the multiply will provably not overflow. |
I generally agree with this. Maybe the one mistake here is that
Maybe we should land #164246 first. |
If operands of multiply are 64 bit values that are sign-extended to 128 bit then the product trivially won't overflow 128-bit computation. I don't see what is the issue being discussed here. To do the comparison we extend either subtraction result or subtraction operands to 128 bits. (extending subtration operands to 128 bit will obviously generate a 128 bit result). That said, there was probably a misinterpretation of the some examples on our side. While the idea is correct, we don't see clear advantage on it over simpler check of nsw flag of the multiply (and probably some other safeguards) , at least for SIV. @1997alireza can change this patch to implement the simpler solution. We can check if the idea is useful for other tests or not. |
Right, when overflow for the product is a possibility that will work. Previously we thought for some simple loops, the product may overflow and we can prevent it. |
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
271f32a to
6482263
Compare
1997alireza
changed the title
amehsan
approved these changes
Oct 27, 2025
There was a problem hiding this comment.
@1997alireza when I checkout your PR locally, in git log I see your email as "a00917109 [email protected]" and your name in git blame appears as this id as well. Your previous commits are OK though. Please check it.
nikic
reviewed
Oct 27, 2025
| ; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \ | ||
| ; RUN: | FileCheck %s | ||
| ; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa -da-enable-dependence-test=strong-siv 2>&1 \ | ||
| ; RUN: | FileCheck %s --check-prefix=CHECK-STRONG-SIV |
There was a problem hiding this comment.
Suggested change
| ; RUN: | FileCheck %s --check-prefix=CHECK-STRONG-SIV | |
| ; RUN: | FileCheck %s --check-prefixes=CHECK,CHECK-STRONG-SIV |
and --check-prefixes=CHECK,CHECK-ALL or something above. To avoid redundant check lines in cases where these are the same.
There was a problem hiding this comment.
And then I'd also suggest pre-committing the test changes, so that this PR shows the diff.
There was a problem hiding this comment.
Thanks. I used the prefixes to avoid the redundant checks. I also added a pre commit to include the test case only, so now in the final commit we can see what has been changed in the test case. Let me know if it looks good to you.
kasuga-fj
reviewed
Oct 27, 2025
|
|
||
| for.cond1.preheader: ; preds = %entry, %for.inc5 | ||
| %i.014 = phi i64 [ 0, %entry ], [ %inc6, %for.inc5 ] | ||
| %arrayidx = getelementptr inbounds nuw i32, ptr %A, i64 %i.014 |
There was a problem hiding this comment.
I think this will always be UB due to violating the size limitation of allocated objects.
There was a problem hiding this comment.
Test case is modified now.
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
6482263 to
77ee86e
Compare
1997alireza
changed the title
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
77ee86e to
ce4f93f
Compare
I fixed the author name. |
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
ce4f93f to
66094b9
Compare
1997alireza
force-pushed
the
strong-siv-overflow
branch
from
66094b9 to
656be5a
Compare
amehsan
approved these changes
Oct 31, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/16449 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/30074 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/27095 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/25698 Here is the relevant piece of the build log for the reference |
|
Hi! This PR breaks several bots. Could you please fix it forward or revert it if it takes longer time to investigate? Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/39058 Here is the relevant piece of the build log for the reference |
|
We are seeing failures in Google toolchain builders as well after this PR. Please consider reverting |
|
Yes, working on the revert
From: Prabhu Rajasekaran ***@***.***
Sent: October 31, 2025 2:39 PM
To: llvm/llvm-project ***@***.***>
Cc: Ehsan Amiri ***@***.***>; State change ***@***.***>
Subject: Re: [llvm/llvm-project] [DA] Check for overflow in strong SIV test (PR #164704)
[https://avatars.githubusercontent.com/u/515853?s=20&v=4]Prabhuk left a comment (llvm/llvm-project#164704)<#164704 (comment)>
We are seeing failures in Google toolchain builders as well after this PR. Please consider reverting
Exit Code: 1
Command Output (stdout):
…--
# RUN: at line 2
/b/s/w/ir/x/w/llvm_build/bin/opt < /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll -disable-output "-passes=print<da>" 2>&1 | /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll --check-prefixes=CHECK,CHECK-ALL
# executed command: /b/s/w/ir/x/w/llvm_build/bin/opt -disable-output '-passes=print<da>'
# executed command: /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll --check-prefixes=CHECK,CHECK-ALL
# RUN: at line 4
/b/s/w/ir/x/w/llvm_build/bin/opt < /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll -disable-output "-passes=print<da>" -da-enable-dependence-test=strong-siv 2>&1 | /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll --check-prefixes=CHECK,CHECK-STRONG-SIV
# executed command: /b/s/w/ir/x/w/llvm_build/bin/opt -disable-output '-passes=print<da>' -da-enable-dependence-test=strong-siv
# executed command: /b/s/w/ir/x/w/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll --check-prefixes=CHECK,CHECK-STRONG-SIV
# .---command stderr------------
# | /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll:25:15: error: CHECK-NEXT: is not on the line after the previous match
# | ; CHECK-NEXT: da analyze - none!
# | ^
# | <stdin>:7:2: note: 'next' match was here
# | da analyze - none!
# | ^
# | <stdin>:4:78: note: previous match ended here
# | Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
# | ^
# | <stdin>:5:1: note: non-matching line after previous match is here
# | da analyze - consistent output [1]!
# | ^
# |
# | Input file: <stdin>
# | Check file: /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/Analysis/DependenceAnalysis/strong-siv-overflow.ll
# |
# | -dump-input=help explains the following input dump.
# |
# | Input was:
# | <<<<<<
# | 1: Printing analysis 'Dependence Analysis' for function 'strongsiv_const_ovfl':
# | 2: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
# | 3: da analyze - none!
# | 4: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
# | 5: da analyze - consistent output [1]!
# | 6: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
# | 7: da analyze - none!
# | next:25 !~~~~~~~~~~~~~~~~~ error: match on wrong line
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1
—
Reply to this email directly, view it on GitHub<#164704 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACIGTT7VQ5HOVCB2XJYJURT32OUDTAVCNFSM6AAAAACJ56M2JKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINZUGM4TKNJUGI>.
You are receiving this because you modified the open/close state.Message ID: ***@***.******@***.***>>
|
google-yfyang
added a commit
to google-yfyang/llvm-project
that referenced
this pull request
Oct 31, 2025
1997alireza
added a commit
to 1997alireza/llvm-project
that referenced
this pull request
Oct 31, 2025
This reverts commit 1b3e7df.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/85/builds/15152 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/14223 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/28013 Here is the relevant piece of the build log for the reference |
CongzheUalberta
pushed a commit
that referenced
this pull request
Oct 31, 2025
Reverts #164704 that broke several built bots.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Oct 31, 2025
Reverts llvm/llvm-project#164704 that broke several built bots.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/27492 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/27632 Here is the relevant piece of the build log for the reference |
DEBADRIBASAK
pushed a commit
to DEBADRIBASAK/llvm-project
that referenced
this pull request
Nov 3, 2025
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`.
DEBADRIBASAK
pushed a commit
to DEBADRIBASAK/llvm-project
that referenced
this pull request
Nov 3, 2025
Reverts llvm#164704 that broke several built bots.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
[EDIT: After the discussion in this PR, we found out no real benefit of extending the expressions rather than dropping the results in case of overflow. Now we check the overflow and rely on the product of
UpperBoundandAbsCoeffonly if SCEV can prove that there is no overflow. Also the same about the result of the subtraction ofDstConstfromSrcConstto calculateDelta.]By doubling the size of the expressions in the strong SIV test, no overflow occurs in the multiplicationSE->getMulExpr(WideUpperBound, WideAbsCoeff)anymore.This patch resolves the issues with the strong SIV test in both #159846 and #164246 by proving the dependency.
While this patch resolves the overflow bug in the strong SIV test, it also prevents the test from performing and disproving the dependency in some cases. The reason behind this is due to addingsextoperation when we are extending the expressions, which prevents SCEV to compare the expressions and disprove the dependency in those special cases. We are working on that. Check out the progress report.