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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe 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));
----------------
bmahjour wrote:
> [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?
> 
Thanks for the comments! 

Size of src and dst subscripts arrays are supposed to be the same if we reach this point. And I totally agree that it is worth adding an assertion to check that. I've added the assert accordingly.

You are right that Src and Dst do not need to be cast to GEPs (we did not gain anything). Now I've removed the cast.


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

https://reviews.llvm.org/D124745