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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula requested changes to this revision.
bondhugula added a comment.
This revision now requires changes to proceed.

This looks like a good refactoring, thanks! Some (mostly minor) comments.



================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:189
 
+/// generateDataCopyAroundOp is similar to affineDataCopyGenerate, but with some
+/// simplifications:
----------------
This doc comment should moved down to the method declaration below. Please mention that this performs data copy generation for a single memref region associated with a single op. 


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:191
+/// simplifications:
+/// * The logic of "find relavant memrefs and their uses" is de-coupled and push
+/// back to the users. It focuses on generating fast buffers and associated
----------------
relevant


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:191
+/// simplifications:
+/// * The logic of "find relavant memrefs and their uses" is de-coupled and push
+/// back to the users. It focuses on generating fast buffers and associated
----------------
bondhugula wrote:
> relevant
push -> pushed


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:198
+///
+/// Notice that certain options in `copyOptions` isn't looked at anymore, like
+/// slowMemorySpace.
----------------
Nit: Notice -> Note that


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:203
+  Operation *alloc;
+  Operation *copyNest;
+};
----------------
Doc comment. 


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:207
+LogicalResult generateDataCopyAroundOp(const MemRefRegion &memrefRegion,
+                                       Operation *where,
+                                       const AffineCopyOptions &copyOptions,
----------------
A better name for 'where'?


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1818
+  assert(copyNests.size() <= 1 &&
+         "Multiple copy nests generated appear for a single memref.");
+  result.copyNest = copyNests.empty() ? nullptr : *copyNests.begin();
----------------
"At most one copy nest expected" ?


================
Comment at: mlir/test/lib/Transforms/TestAffineDataCopy.cpp:43
+      *this, "test-generate-data-copy-around-op",
+      llvm::cl::desc("Test through generateCopyAroundOp"),
+      llvm::cl::init(false)};
----------------
Test copy generation for a single op / memref region?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75965