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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:189
 
+struct CopyGenerateResult {
+  // Number of bytes used by alloc.
----------------
Doc comment here please (even if it's brief).


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:194
+  // The newly created buffer.
+  Operation *alloc;
+
----------------
buffer -> buffer allocation


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:196
+
+  // Generated loop nest for copying data between `alloc` and the original
+  // memref.
----------------
`alloc` above is an operation. alloc -> allocated buffer?


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:201
+
+/// generateCopyFromMemRefRegion is similar to affineDataCopyGenerate, but with
+/// some simplifications:
----------------
generateCopyFromMemRefRegion -> generateCopyForMemRegion ?


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:215
+LogicalResult generateCopyFromMemRefRegion(const MemRefRegion &memrefRegion,
+                                           Operation *insertion_point,
+                                           const AffineCopyOptions &copyOptions,
----------------
Camel case. 


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:215
+LogicalResult generateCopyFromMemRefRegion(const MemRefRegion &memrefRegion,
+                                           Operation *insertion_point,
+                                           const AffineCopyOptions &copyOptions,
----------------
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). 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75965