[PATCH] D78996: [MLIR][LINALG] Convert Linalg to Linalg

[Ehsan via Phabricator via llvm-commits]

dfki-ehna marked 5 inline comments as done.
dfki-ehna added a comment.

In D78996#2007630 <https://reviews.llvm.org/D78996#2007630>, @ftynse wrote:

> `lib/Conversion` is intended to for conversions from dialect A to dialect B to ensure it can depend on both without creating unnecessary dependencies `A->B` or `B->A`.  Since this transformation stays within Linalg, it should go to `lib/Dialect/Linalg/Transforms`. Beyond being cleaner library-wise, it can have a more meaningful name than "linalg to linalg", e.g. "TensorsToBuffers" or "BufferAssignment".


Thanks. The files were moved to the correct locations.



================
Comment at: mlir/lib/Conversion/LinalgToLinalg/LinalgToLinalg.cpp:74
+
+    rewriter.eraseOp(op);
+    return success();
----------------
pifon2a wrote:
> shouldn't it be `rewriter.replaceOp(op, <what to replace results with>)`? Please, add a test with two chained Linalg ops.
Thanks and resolved. We were replacing the results' uses without rewriter using replaceAllUsesWith which was unsafe.


================
Comment at: mlir/lib/Conversion/LinalgToLinalg/LinalgToLinalg.cpp:103
+
+void TensorLinalgToBufferLinalgPass::runOnOperation() {
+  auto &context = getContext();
----------------
pifon2a wrote:
> why is the definition of `runOnOperation()` method not inside `TensorLinalgToBufferLinalgPass`? I don't think it improved readability here. Is there some guide that does not allow doing that?
Resolved. The reason for doing this was consistency with some other CPP files in the lib/conversion directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78996