[PATCH] D77727: Only insert memref_cast when needed during canonicalization.

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

Please include a brief commit summary. I only did a superficial review. Could you please include test cases?



================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:312
 namespace {
+// Create a memref_cast when needed to convert from newType to the type of
+// oldOp. Called when canonilizing a view or subview op that changed the type of
----------------
Doc comment with ///


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:313
+// Create a memref_cast when needed to convert from newType to the type of
+// oldOp. Called when canonilizing a view or subview op that changed the type of
+// the memref (from a dynamic-sized memref to a statically-sized memref). Only
----------------
Typo


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:322
+  auto uses = oldOp->getUses();
+  for (auto it = uses.begin(); it != uses.end(); it++) {
+    auto *user = it.getUser();
----------------
More compact to use: 

`for (auto *userOp : oldOp->getUsers())`


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:329
+  if (canSkipMemrefCast) {
+    for (auto it = uses.begin(); it != uses.end(); it++)
+      it->get().setType(newValue.getType());
----------------
Likewise. 


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:335
+                                                oldValue.getType());
+    rewriter.replaceOp(oldOp, {castOp});
+  }
----------------
You could use `replaceOpWithNewOp(oldOp, oldValue.getLoc(), ...)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77727