[PATCH] D124745: [Delinearization] Refactoring of fixed-size array delinearization

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/Delinearization.cpp:540
+  if (Sizes.empty() || Subscripts.size() <= 1) {
+    Subscripts.clear();
+    return false;
----------------
congzhe wrote:
> Meinersbur wrote:
> > `Subscripts.clear()` should not be necessary, the caller is supposed to throw it away if returning `false`. We actually know that it is empty because `getIndexExpressionsFromGEP` has an assert for it. At least it is consistent  that `Sizes` is not cleared too.
> Thanks for the comment! I tried to remove `Subscripts.clear();` but it failed a few lit tests: 
> ```
>   LLVM :: Analysis/DDG/basic-loopnest.ll
>   LLVM :: Analysis/DependenceAnalysis/Coupled.ll
>   LLVM :: Analysis/DependenceAnalysis/DADelin.ll
>   LLVM :: Analysis/DependenceAnalysis/SimpleSIVNoValidityCheck.ll
>   LLVM :: Analysis/LoopCacheAnalysis/PowerPC/compute-cost.ll
>   LLVM :: Analysis/LoopCacheAnalysis/PowerPC/loads-store.ll
>   LLVM :: Analysis/LoopCacheAnalysis/PowerPC/matmul.ll
>   LLVM :: Analysis/LoopCacheAnalysis/PowerPC/single-store.ll
>   LLVM :: Analysis/LoopCacheAnalysis/PowerPC/stencil.ll
> ```
> 
> IMHO we may need to keep `Subscripts.clear()`. Different from `Sizes` which indeed gets cleared/thrown away by the caller (e.g., `DependenceInfo::tryDelinearizeFixedSize()`), `Subscripts` does not get cleared by `tryDelinearizeFixedSize()` (if it has only one element), and will be passed into `DependenceInfo::tryDelinearizeParametricSize()` and causes incorrect result. 
> 
> There might be ways to further modify other places which would enable us to remove this ` Subscripts.clear();`, but I thought we might keep it and avoid too much modification?
It's something to be improved. I don't like the inconsistent handling when invalid. Either only return `false` and the caller is expected to handle this, or we unconditionally clear the array at the beginning so we know there is no junk data from a previous call.

Could you add a `TODO`?


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

https://reviews.llvm.org/D124745