[PATCH] D75965: [mlir] Add a simplifying wrapper for generateCopy and expose it.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:201
+
+/// generateCopyFromMemRefRegion is similar to affineDataCopyGenerate, but with
+/// some simplifications:
----------------
bondhugula wrote:
> bondhugula wrote:
> > generateCopyFromMemRefRegion -> generateCopyForMemRegion ?
> This doc comment appears to provides detail before the actual information. Perhaps just rephrase to:
> 
> /// Similar to affineDataCopyGenerate, but works on a single memref region.
> 
> You could move "The logic of "find ..." as a comment on the definition of the method. 
Simplified further. The "The logic of" part looks like design rationales rather than function descriptions, which is repeating after the API itself.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:215
+LogicalResult generateCopyFromMemRefRegion(const MemRefRegion &memrefRegion,
+                                           Operation *insertion_point,
+                                           const AffineCopyOptions &copyOptions,
----------------
bondhugula wrote:
> bondhugula wrote:
> > Camel case. 
> insertionPoint would be used typically for iterator types. Shouldn't this just be a Block::iterator? At the call site, you could pass Block::iterator(op). 
This op is more than an iterator insertion point. It's also the op that's analyzed by the memref region. It also indicate that something next to the iterator is going to change (std::next(insertionPoint)). Change the name to analyzedOp, and adjusted comments for that.


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1803-1804
+    const AffineCopyOptions &copyOptions, CopyGenerateResult &result) {
+  Block *block = insertion_point->getBlock();
+  auto begin = insertion_point->getIterator();
+  auto end = std::next(begin);
----------------
bondhugula wrote:
> Block::iterator(insertion_point) would have given you the iterator. But you don't need this anyway if you pass a Block::iterator itself to this method. 
N/A.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75965