[PATCH] D122857: [LoopCacheAnalysis] Enable delinearization of fixed sized arrays
Congzhe Cao via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 13 20:11:47 PDT 2022
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
More information about the llvm-commits
mailing list