-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[DA] Fix Strong SIV test for symbolic coefficients and deltas (#149977) #157738
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
Open
sebpop
wants to merge
1
commit into
llvm:users/sebpop/da-run-predicates
Choose a base branch
from
sebpop:pr149977
base: users/sebpop/da-run-predicates
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+111
−12
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1249,10 +1249,33 @@ bool DependenceInfo::strongSIVtest(const SCEV *Coeff, const SCEV *SrcConst, | |
| SE->isKnownNonNegative(Coeff) ? Coeff : SE->getNegativeSCEV(Coeff); | ||
| const SCEV *Product = SE->getMulExpr(UpperBound, AbsCoeff); | ||
| if (isKnownPredicate(CmpInst::ICMP_SGT, AbsDelta, Product)) { | ||
| // Distance greater than trip count - no dependence | ||
| ++StrongSIVindependence; | ||
| ++StrongSIVsuccesses; | ||
| return true; | ||
| // Check if this involves symbolic expressions where we might be too | ||
| // conservative. | ||
| if (isa<SCEVUnknown>(Delta) || isa<SCEVUnknown>(Coeff) || | ||
| !isa<SCEVConstant>(AbsDelta) || !isa<SCEVConstant>(Product)) { | ||
| // For symbolic expressions, add runtime assumption rather than | ||
| // rejecting. | ||
| const SCEVPredicate *BoundPred = | ||
| SE->getComparePredicate(ICmpInst::ICMP_SLE, AbsDelta, Product); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. push this inside if(){...} |
||
| if (UnderRuntimeAssumptions) { | ||
| SmallVector<const SCEVPredicate *, 4> NewPreds( | ||
| Assumptions.getPredicates()); | ||
| NewPreds.push_back(BoundPred); | ||
| const_cast<DependenceInfo *>(this)->Assumptions = | ||
| SCEVUnionPredicate(NewPreds, *SE); | ||
| LLVM_DEBUG(dbgs() << "\t Added runtime bound assumption\n"); | ||
| } else { | ||
| // Cannot add runtime assumptions, let more complex tests try. | ||
| LLVM_DEBUG(dbgs() << "\t Would need runtime bound assumption but " | ||
| "not allowed. Failing this test.\n"); | ||
| return false; | ||
| } | ||
| } else { | ||
| // Distance definitely greater than trip count - no dependence | ||
| ++StrongSIVindependence; | ||
| ++StrongSIVsuccesses; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1293,9 +1316,40 @@ bool DependenceInfo::strongSIVtest(const SCEV *Coeff, const SCEV *SrcConst, | |
| Result.DV[Level].Distance = Delta; // since X/1 == X | ||
| NewConstraint.setDistance(Delta, CurLoop); | ||
| } else { | ||
| Result.Consistent = false; | ||
| NewConstraint.setLine(Coeff, SE->getNegativeSCEV(Coeff), | ||
| SE->getNegativeSCEV(Delta), CurLoop); | ||
| // Try symbolic division: Distance = Delta / Coeff. | ||
| if (const SCEV *Distance = SE->getUDivExactExpr(Delta, Coeff)) { | ||
| LLVM_DEBUG(dbgs() << "\t Symbolic distance = " << *Distance << "\n"); | ||
| Result.DV[Level].Distance = Distance; | ||
| NewConstraint.setDistance(Distance, CurLoop); | ||
| } else { | ||
| // Cannot compute exact division - check if we can add runtime | ||
| // assumptions. | ||
| if (isa<SCEVUnknown>(Coeff) && !SE->isKnownNonZero(Coeff)) { | ||
| // Add runtime assumption that coefficient is non-zero for division. | ||
| const SCEV *Zero = SE->getZero(Coeff->getType()); | ||
| const SCEVPredicate *NonZeroPred = | ||
| SE->getComparePredicate(ICmpInst::ICMP_NE, Coeff, Zero); | ||
| if (UnderRuntimeAssumptions) { | ||
| SmallVector<const SCEVPredicate *, 4> NewPreds( | ||
| Assumptions.getPredicates()); | ||
| NewPreds.push_back(NonZeroPred); | ||
| const_cast<DependenceInfo *>(this)->Assumptions = | ||
| SCEVUnionPredicate(NewPreds, *SE); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we adding constraint once again when we have already added below constraint? |
||
| LLVM_DEBUG(dbgs() << "\t Added runtime assumption: " << *Coeff | ||
| << " != 0 for symbolic division\n"); | ||
| } else { | ||
| // Cannot add runtime assumptions, this test fails. | ||
| LLVM_DEBUG(dbgs() | ||
| << "\t Would need runtime assumption " << *Coeff | ||
| << " != 0 but not allowed. Failing this test.\n"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Result.Consistent = false; | ||
| NewConstraint.setLine(Coeff, SE->getNegativeSCEV(Coeff), | ||
| SE->getNegativeSCEV(Delta), CurLoop); | ||
| } | ||
| } | ||
|
|
||
| // maybe we can get a useful direction | ||
|
|
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5 | ||
| ; RUN: opt < %s -disable-output "-passes=print<da>" 2>&1 | FileCheck %s | ||
|
|
||
| ; Test case for GitHub issue #149977: Strong SIV test with symbolic coefficients and deltas | ||
| ; The issue was that the bound constraint check was overly conservative with symbolic expressions, | ||
| ; causing valid dependencies to be rejected before reaching the division logic. | ||
| ; | ||
| ; Mathematical analysis: | ||
| ; - Access patterns: a[-k*i] vs a[-k*i + (2*k + 1)] | ||
| ; - Strong SIV equation: -k*(i2-i1) = (2*k + 1) | ||
| ; - Distance: (2*k + 1)/k | ||
| ; - For k=-1: distance = -1/-1 = 1 (clear dependence) | ||
|
|
||
| define void @f(ptr %a, i64 %k) { | ||
| ; CHECK-LABEL: 'f' | ||
| ; CHECK-NEXT: Src: store i8 42, ptr %idx.0, align 1 --> Dst: store i8 42, ptr %idx.0, align 1 | ||
| ; CHECK-NEXT: da analyze - none! | ||
| ; CHECK-NEXT: Src: store i8 42, ptr %idx.0, align 1 --> Dst: store i8 42, ptr %idx.1, align 1 | ||
| ; CHECK-NEXT: da analyze - consistent output [((-1 + (-2 * %k)) /u (-1 * %k))]! | ||
| ; CHECK-NEXT: Runtime Assumptions: | ||
| ; CHECK-NEXT: Compare predicate: (1 + (2 * %k))<nuw><nsw> sle) (2 * %k) | ||
| ; CHECK-NEXT: Src: store i8 42, ptr %idx.1, align 1 --> Dst: store i8 42, ptr %idx.1, align 1 | ||
| ; CHECK-NEXT: da analyze - none! | ||
| ; | ||
| entry: | ||
| %mk = sub i64 0, %k ; mk = -k | ||
| %kk = mul i64 %k, 2 ; kk = 2*k | ||
| %kk.inc = add i64 1, %kk ; kk.inc = 2*k + 1 | ||
| br label %loop | ||
|
|
||
| loop: | ||
| %i = phi i64 [ 0, %entry ], [ %i.next, %loop ] | ||
| %subscript.0 = mul i64 %mk, %i ; -k * i | ||
| %subscript.1 = add i64 %subscript.0, %kk.inc ; -k * i + (2*k + 1) | ||
| %idx.0 = getelementptr i8, ptr %a, i64 %subscript.0 | ||
| %idx.1 = getelementptr i8, ptr %a, i64 %subscript.1 | ||
| store i8 42, ptr %idx.0 | ||
| store i8 42, ptr %idx.1 | ||
| %i.next = add i64 %i, 1 | ||
| %cond.exit = icmp eq i64 %i.next, 3 | ||
| br i1 %cond.exit, label %exit, label %loop | ||
|
|
||
| exit: | ||
| ret void | ||
| } |
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
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
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.
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.
should be sufficient ?