-
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 3 commits
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 proven by induction) | ||||||
| // | ||||||
| const SCEV *Prod = SE.getOne(Sizes[0]->getType()); | ||||||
| for (const SCEV *Size : Sizes) { | ||||||
| Prod = MulOverflow(Prod, Size); | ||||||
| if (!Prod) | ||||||
| return false; | ||||||
| } | ||||||
| const SCEV *Min = MulOverflow(Prod, Subscripts[0]); | ||||||
|
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. 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 (
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. I'm not sure until we try it, but once a
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. Does the following implementation match your intention? As for the existing regression tests, their results have not changed.
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.
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.
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. 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. |
||||||
| if (!Min) | ||||||
| return false; | ||||||
|
|
||||||
| // Over-approximate Max as Prod * I_1 + Prod (ignoring the -1). | ||||||
| if (!SE.willNotOverflow(Instruction::Add, /*IsSigned=*/true, Min, | ||||||
| Subscripts[0])) | ||||||
| return false; | ||||||
|
|
||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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.