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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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:
> 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.


================
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.
----------------
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.


================
Comment at: llvm/lib/Analysis/Delinearization.cpp:528
+  Value *SrcPtr = getLoadStorePointerOperand(Inst);
+  const SCEVUnknown *SrcBase = cast<SCEVUnknown>(SE->getPointerBase(AccessFn));
+
----------------
Cannot assume that `getPointerBase` returns a `SCEVUnknown`.

`SrcBase` is only used down below, consider moving it there.


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


================
Comment at: llvm/lib/Analysis/Delinearization.cpp:549
+  if (SrcBasePtr != SrcBase->getValue()) {
+    Subscripts.clear();
+    return false;
----------------
See above


================
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.
----------------
[nit]


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

https://reviews.llvm.org/D124745