-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Delinearization] Add validation for large size arrays #169902
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
base: users/kasuga-fj/delinearize-add-validation-test
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -747,6 +747,20 @@ bool llvm::validateDelinearizationResult(ScalarEvolution &SE, | |||||
| ArrayRef<const SCEV *> Sizes, | ||||||
| ArrayRef<const SCEV *> Subscripts, | ||||||
| const Value *Ptr) { | ||||||
| // Sizes and Subscripts are as follows: | ||||||
| // | ||||||
| // Sizes: [UNK][S_2]...[S_n] | ||||||
| // Subscripts: [I_1][I_2]...[I_n] | ||||||
| // | ||||||
| // where the size of the outermost dimension is unknown (UNK). | ||||||
|
|
||||||
| auto MulOverflow = [&](const SCEV *A, const SCEV *B) -> const SCEV * { | ||||||
| if (!SE.willNotOverflow(Instruction::Mul, /*IsSigned=*/true, A, B)) | ||||||
| return nullptr; | ||||||
| return SE.getMulExpr(A, B); | ||||||
| }; | ||||||
|
|
||||||
| // Range check: 0 <= I_k < S_k for k = 2..n. | ||||||
| for (size_t I = 1; I < Sizes.size(); ++I) { | ||||||
| const SCEV *Size = Sizes[I - 1]; | ||||||
| const SCEV *Subscript = Subscripts[I]; | ||||||
|
|
@@ -755,6 +769,43 @@ bool llvm::validateDelinearizationResult(ScalarEvolution &SE, | |||||
| if (!isKnownLessThan(&SE, Subscript, Size)) | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| // The offset computation is as follows: | ||||||
| // | ||||||
| // Offset = I_n + | ||||||
| // S_n * I_{n-1} + | ||||||
| // ... + | ||||||
| // (S_2 * ... * S_n) * I_1 | ||||||
| // | ||||||
| // Regarding this as a function from (I_1, I_2, ..., I_n) to integers, it | ||||||
| // must be injective. To guarantee it, the above calculation must not | ||||||
| // overflow. Since we have already checked that 0 <= I_k < S_k for k = 2..n, | ||||||
| // the minimum and maximum values occur in the following cases: | ||||||
| // | ||||||
| // Min = [I_1][0]...[0] = S_2 * ... * S_n * I_1 | ||||||
| // Max = [I_1][S_2-1]...[S_n-1] | ||||||
| // = (S_2 * ... * S_n) * I_1 + | ||||||
| // (S_2 * ... * S_{n-1}) * (S_2 - 1) + | ||||||
| // ... + | ||||||
| // (S_n - 1) | ||||||
| // = (S_2 * ... * S_n) * I_1 + | ||||||
|
Member
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.
Suggested change
Some cleverness I did not see (
Contributor
Author
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. Adjusted the comments a bit. Does it make sense? |
||||||
| // (S_2 * ... * S_n) - 1 (can be proved by induction) | ||||||
kasuga-fj marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| // | ||||||
| const SCEV *Prod = SE.getOne(Sizes[0]->getType()); | ||||||
| for (const SCEV *Size : drop_end(Sizes)) { | ||||||
|
||||||
| for (const SCEV *Size : drop_end(Sizes)) { | |
| for (const SCEV *Size : drop_begin(Sizes)) { |
Shouldn't this skip the first dimension S_1 that we do not know the size of?
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.
It's a bit ugly, but currently Sizes has N elements. The first N-1 elements are S_2, S_3, ..., S_{N-1}, and the last element is ElementSize, which represents the size of the data for the operand of the memory instruction being analyzed.
While writing this comment, I'm starting to think it should not be dropped...
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.
Removed drop_end as I found it's not correct. Also added one more test case.
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.
For the first dimension, it takes the concrete subscript, for every other dimension it assumes the largest possible subscript. Would we get a better estimate combining both sources of knowledge by the max of those (SCEVMaxExpr(Subscripts[i],Size[i]-1))?
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.
I'm not sure until we try it, but once a SCEVUnknown gets in there, I feel like it probably won't change much.
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.
Does the following implementation match your intention?
As for the existing regression tests, their results have not changed.
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.
Does the following implementation match your intention?
Yes.
If one is SCEVUnknown, one can take the other one. Only both being SCEVUnknown would stop working. if SCEVMaxExprs cause SCEV to bail out because of expression complexity, we cioud prioritize one of the expressions.
Aynway, what I prefer handling subscripts independent of whether they are outermost, innermost, or in-between, but based on what information is available.
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.
Ah, I see, I understand your point. I’m not sure if there are cases where this actually improves things, but in point of implementation, it seems more consistent.
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.
Why isn't Min just 0 (
I_1 == 0)?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.
Because DA appears to allow the outermost subscript to be negative, e.g.,
llvm-project/llvm/test/Analysis/DependenceAnalysis/Propagating.ll
Lines 310 to 314 in 318236d
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.
Would be worth adding a comment?
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.
Yes, added.