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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/IR/PatternMatch.h:285
+  /// known to have no uses.
+  virtual void eraseBlock(Block *block);
+
----------------
Can you add proper handling for DialectConversion?


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


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:1580
+
+    rewriter.startRootUpdate(ifOp);
+    rewriter.eraseBlock(ifOp.getElseBlock());
----------------
nit: 
```
rewriter.updateRootInPlace(ifOp, [&] {
  rewriter.eraseBlock(if.getElseBlock());
});
```

?


================
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);
----------------
This seems wrong, any non-trivially empty block will likely fail this check. Did you want to iterate the block in reverse?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77083