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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3349
 
-  Value *SrcBasePtr = SrcGEP->getOperand(0)->stripPointerCasts();
-  Value *DstBasePtr = DstGEP->getOperand(0)->stripPointerCasts();
-
-  // Check that for identical base pointers we do not miss index offsets
-  // that have been added before this GEP is applied.
-  if (SrcBasePtr != SrcBase->getValue() || DstBasePtr != DstBase->getValue()) {
-    SrcSubscripts.clear();
-    DstSubscripts.clear();
-    return false;
-  }
-
-  assert(SrcSubscripts.size() == DstSubscripts.size() &&
-         SrcSubscripts.size() == SrcSizes.size() + 1 &&
-         "Expected equal number of entries in the list of sizes and "
-         "subscripts.");
+  /// At this point we know \p SrcPtr is a \p GetElementPtrInst.
+  Value *SrcGEP = cast<GetElementPtrInst>(getLoadStorePointerOperand(Src));
----------------
[nit] '\p' is used to refer to function parameters when commenting on a function. 
[nit] `///` is not meant for comments inside a function.
[nit] SrcPtr -> Src
[nit] `Src` is mentioned but not `Dst`
[suggestion] assert that size of src and dst subscripts arrays are the same.
[comment] it would be better to assert that Src and Dst are GEP before casting, and SrcGEP and DstGEP should be typed as GEP instead of 'Value'. What do we gain by assuming that the Src and Dst are GEPs anyway?



================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:329
 
-  SmallVector<int, 4> SrcSizes;
-  getIndexExpressionsFromGEP(*SE, SrcGEP, SrcSubscripts, SrcSizes);
-
-  // Check that the two size arrays are non-empty and equal in length and
-  // value.
-  if (SrcSizes.empty() || SrcSubscripts.size() <= 1) {
-    SrcSubscripts.clear();
-    return false;
-  }
-
-  Value *SrcBasePtr = SrcGEP->getOperand(0)->stripPointerCasts();
-
-  // Check that for identical base pointers we do not miss index offsets
-  // that have been added before this GEP is applied.
-  if (SrcBasePtr != SrcBase->getValue()) {
-    SrcSubscripts.clear();
-    return false;
-  }
-
-  assert(SrcSubscripts.size() == SrcSizes.size() + 1 &&
-         "Expected equal number of entries in the list of size and "
-         "subscript.");
-
+  /// Populate \p Sizes with scev expressions to be used in calculations later.
   for (auto Idx : seq<unsigned>(1, Subscripts.size()))
----------------
[nit] use `//` for function internal comments


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:364
     bool IsFixedSize = false;
-    // Try to delinearize fixed-size arrays.
-    if (tryDelinearizeFixedSize(&SE, &StoreOrLoadInst, AccessFn, Subscripts)) {
+    /// Try to delinearize fixed-size arrays.
+    if (tryDelinearizeFixedSize(AccessFn, Subscripts)) {
----------------
[nit] use `//` for function internal comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124745