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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/include/llvm/Analysis/Delinearization.h:128-129
 
+/// Implementation of fixed size array delinearization, can be reused
+/// wherever needed with the llvm namespace. Tries to delinearize access
+/// function for a fixed size multi-dimensional array, by deriving subscripts
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > Could you explain the relation between `Inst` and `AccessFn`? One of them seems redundant because of the `SrcBasePtr != SrcBase->getValue()` check, i.e. one can be derived from the other.
> "can be reused wherever needed with the llvm namespace" does not need documentation.
Thanks, I've updated it accordingly.


================
Comment at: llvm/include/llvm/Analysis/Delinearization.h:128-131
+/// Implementation of fixed size array delinearization, can be reused
+/// wherever needed with the llvm namespace. Tries to delinearize access
+/// function for a fixed size multi-dimensional array, by deriving subscripts
+/// from GEP instructions. Returns true upon success and false otherwise.
----------------
congzhe wrote:
> Meinersbur wrote:
> > Meinersbur wrote:
> > > Could you explain the relation between `Inst` and `AccessFn`? One of them seems redundant because of the `SrcBasePtr != SrcBase->getValue()` check, i.e. one can be derived from the other.
> > "can be reused wherever needed with the llvm namespace" does not need documentation.
> Thanks, I've updated it accordingly.
Thanks, I've added some explanations about `Inst` and `AccessFn` here in the comment. Generally speaking `AccessFn` can indeed be derived from `Inst`, i.e., `AccessFn = SE.getSCEVAtScope(getPointerOperand(Inst), LI->getLoopFor(Inst->getParent()));`. It is not immediately doable to remove the arugment `AccessFn` though, because we would need `LoopInfo *LI` to be passed here in order to derive `AccessFn` from `Inst`, which brings another aspect of complexity. So I left the function declaration as it is for now.


================
Comment at: llvm/lib/Analysis/Delinearization.cpp:528
+  Value *SrcPtr = getLoadStorePointerOperand(Inst);
+  const SCEVUnknown *SrcBase = cast<SCEVUnknown>(SE->getPointerBase(AccessFn));
+
----------------
Meinersbur wrote:
> Cannot assume that `getPointerBase` returns a `SCEVUnknown`.
> 
> `SrcBase` is only used down below, consider moving it there.
Thanks, I've used `SrcBase  = dyn_cast<SCEVUnknown>(getPointerBase)` instead, and made it bail out if `SrcBase` is `NULL`.


================
Comment at: llvm/lib/Analysis/Delinearization.cpp:540
+  if (Sizes.empty() || Subscripts.size() <= 1) {
+    Subscripts.clear();
+    return false;
----------------
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?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3319
+/// Try to delinearize \p SrcAccessFn and \p DstAccessFn if the underlying
+/// arrays accessed are fixed-size arrays. Return true if delinearization is
+/// successful.
----------------
Meinersbur wrote:
> [nit]
Thanks, fixed it.


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

https://reviews.llvm.org/D124745