[PATCH] D122857: [LoopCacheAnalysis] Enable delinearization of fixed sized arrays

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopCacheAnalysis.h:146-147
+
+  /// True if the underlying array is not parametric-sized.
+  bool IsFixedSize = false;
 };
----------------
bmahjour wrote:
> Meinersbur wrote:
> > In principle, each dimension can be dynamic or fixed size separately. For instance using C99 VLAs:
> > ```
> > void func(int n, double A[][128][n]);
> > ```
> > has 3 dimensions of which only the middle one is fixed size. However, DependenceInfo may not support them either?
> The existing delinearization algorithms only work if all sizes are parametric or if all are constant. `A` in the example above will be a plain `double* ` pointer, so `getIndexExpressionsFromGEP()` won't be able to recover much from it. 
Thanks for the explanation, that is exactly what would happen.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:355
+
+  for (unsigned i = 1; i < Subscripts.size(); i++)
+    Sizes.push_back(SE->getConstant(Subscripts[i]->getType(), SrcSizes[i - 1]));
----------------
bmahjour wrote:
> can use a range-based loop here
Please correct me if I'm wrong -- here we need the index (i) to index into both `Subscripts` and `SrcSizes`. If we used a range-based loop like `for (const SCEV *S : Subscripts)`, we would not be able to get the elements from `SrcSizes`?


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:358
+
+  assert(SrcSubscripts.size() == SrcSizes.size() + 1 &&
+         "Expected equal number of entries in the list of size and "
----------------
bmahjour wrote:
> [nit] this assert should be done before the copy into Sizes.
Thanks for the comment, I've placed the assert before the copy into Sizes.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:392
+    if (tryDelinearizeFixedSize(&SE, &StoreOrLoadInst, AccessFn, Subscripts)) {
+      IsFixedSize = true;
+      /// The last element of \p Sizes is the element size.
----------------
bmahjour wrote:
> Why do we need to store `IsFixedSize` as a data member?
Thanks, you are right that it does not necessarily need to be a class member. I've updated it to be a local variable.


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

https://reviews.llvm.org/D122857