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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline 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) {
----------------
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.


================
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];
----------------
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*).


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3383
+  }
+  assert(SrcSubscripts.size() == DstSubscripts.size() &&
+         SrcSubscripts.size() == SrcSizes.size() + 1 &&
----------------
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.


================
Comment at: llvm/test/Analysis/DependenceAnalysis/Invariant.ll:8
 
+; float foo (float g, float* rr[40]) {
+;     float res= 0.0f;
----------------
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)
    ...
```


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

https://reviews.llvm.org/D101486