[PATCH] D101486: [Dependence Analysis] Enable delinearization of fixed sized arrays

[Artem Radzikhovskyy via Phabricator via llvm-commits]

artemrad marked an inline comment as done.
artemrad added a comment.

Addressed all comments



================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3350
   }
-
-  SrcSubscripts.clear();
-  DstSubscripts.clear();
-  return false;
+  // Statically check that the array bounds are in-range.
+  if (!DisableDelinearizationChecks) {
----------------
bmahjour wrote:
> perhaps part of the old comment can be carried over here to explain what we mean by in-range and why we have that option.
Done


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3354
+    size_t SrcSize = SrcSubscripts.size();
+    for (size_t I = 1; I < SrcSize && !FailedRangeCheck; ++I) {
+      const auto *S = SrcSubscripts[I];
----------------
Meinersbur wrote:
> bmahjour wrote:
> > In order to avoid code duplication, you should create a lambda for the code from line 3354 to 3364, parameterize `SrcSize`, `SrcPtr`, and `SrcSubscripts`, and call that lambda twice (once for checking Src* and once for checking Dst*).
> A lambda might also useful to have the cleanup code just once. (and make `return` skip all remaining tests)
> ```
>   Check = [] {
>     for (...) {
>       return false;
>     }
>     return true;
>   };
>   if (Check()) {
>      ...
>      return false;
>   }
> 
>   SrcSubscripts.clear();
>   DstSubscripts.clear();
>   return false;
> ```
Done. Thanks for noticing this. It looks much cleaner


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3355
+    for (size_t I = 1; I < SrcSize && !FailedRangeCheck; ++I) {
+      const auto *S = SrcSubscripts[I];
+      if (!isKnownNonNegative(S, SrcPtr))
----------------
Meinersbur wrote:
> [style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Don’t “almost always” use auto ]], except in specific cases.
Done


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3367
+    for (size_t I = 1; I < DstSize && !FailedRangeCheck; ++I) {
+      const auto *S = DstSubscripts[I];
+      if (!isKnownNonNegative(S, DstPtr))
----------------
Meinersbur wrote:
> [style] [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Don’t “almost always” use auto ]], except in specific cases.
Done


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3383
+  }
+  assert(SrcSubscripts.size() == DstSubscripts.size() &&
+         SrcSubscripts.size() == SrcSizes.size() + 1 &&
----------------
bmahjour wrote:
> this assert should be moved right before line 3350, since the new range check assumes that the size array is one element smaller than the subscripts array.
Done


================
Comment at: llvm/test/Analysis/DependenceAnalysis/Invariant.ll:8
 
+; float foo (float g, float* rr[40]) {
+;     float res= 0.0f;
----------------
bmahjour wrote:
> Glad to see this comment, but it should be written as a loop nest to be semantically equivalent to the test
> ```
> for (int i = 0; i < 40; i+=5)
>   for (int j = 0; j < 40; j+=5)
>     ...
> ```
Derp. You are right. Fixed. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101486/new/

https://reviews.llvm.org/D101486