-
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 all 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 + | ||
| // (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. |
||
| 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