[PATCH] D77083: [MLIR] Add pattern rewriter util to erase block; add affine.if pattern to remove dead else blocks

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 8 inline comments as done.
bondhugula added inline comments.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:285
+  /// known to have no uses.
+  virtual void eraseBlock(Block *block);
+
----------------
rriddle wrote:
> Can you add proper handling for DialectConversion?
Sorry, I didn't understand. What does dialect conversion need from eraseBlock? and which it doesn't from eraseOp?


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:1577
+    if (ifOp.elseRegion().empty() ||
+        ifOp.getElseBlock()->getOperations().size() > 1)
+      return failure();
----------------
rriddle wrote:
> Please use has_single_element instead. size() is O(N).
Thanks. 


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:1580
+
+    rewriter.startRootUpdate(ifOp);
+    rewriter.eraseBlock(ifOp.getElseBlock());
----------------
rriddle wrote:
> nit: 
> ```
> rewriter.updateRootInPlace(ifOp, [&] {
>   rewriter.eraseBlock(if.getElseBlock());
> });
> ```
> 
> ?
Thanks, but I tend to prefer the way it is - I find it much simpler to read given the single line action.


================
Comment at: mlir/lib/IR/PatternMatch.cpp:94
+  for (auto &op : llvm::make_early_inc_range(*block)) {
+    assert(op.use_empty() && "expected 'op' to have no uses");
+    notifyOperationRemoved(&op);
----------------
rriddle wrote:
> This seems wrong, any non-trivially empty block will likely fail this check. Did you want to iterate the block in reverse?
Sorry, I didn't understand - what's a non-trivially empty block? AFAICT, the block is either empty of ops or it isn't. 


================
Comment at: mlir/lib/IR/PatternMatch.cpp:96
+    notifyOperationRemoved(&op);
+    op.erase();
+  }
----------------
rriddle wrote:
> Why not just replace this loop with 'eraseOp'?
Sure. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77083