[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);
+
----------------
bondhugula wrote:
> 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?
DialectConversion doesn't allow performing the IR mutation until after the conversion is successful. In this case, you can't erase the block directly because this action may need to be undone later.


================
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);
----------------
bondhugula wrote:
> 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. 
If you have something like:

^bb:
%cst = constant 0 : i64
foo.yield %cst : i64

This will assert when trying to erase the block. Why should we assert that all ops have no uses?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77083