This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by bondhugula on Mar 30 2020, 12:51 PM.

Details

Summary

Add a pattern rewriter utility to erase blocks (while notifying the
pattern rewriting driver of the erased ops). Use this to remove trivial
else blocks in affine.if ops.

Signed-off-by: Uday Bondhugula <uday@polymagelabs.com>

Diff Detail

Event Timeline

bondhugula created this revision.Mar 30 2020, 12:51 PM
Herald added a project: Restricted Project. · View Herald Transcript
bondhugula retitled this revision from [MLIR] Add pattern rewriter util to erase block; remove dead else to [MLIR] Add pattern rewriter util to erase block; add affine.if pattern to remove dead else blocks.Mar 30 2020, 12:53 PM
bondhugula edited the summary of this revision. (Show Details)
rriddle requested changes to this revision.Mar 30 2020, 12:58 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
285

Can you add proper handling for DialectConversion?

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1577

Please use has_single_element instead. size() is O(N).

1580

nit:

rewriter.updateRootInPlace(ifOp, [&] {
  rewriter.eraseBlock(if.getElseBlock());
});

?

mlir/lib/IR/PatternMatch.cpp
94

This seems wrong, any non-trivially empty block will likely fail this check. Did you want to iterate the block in reverse?

96

Why not just replace this loop with 'eraseOp'?

This revision now requires changes to proceed.Mar 30 2020, 12:58 PM
bondhugula marked 8 inline comments as done.Mar 30 2020, 1:23 PM
bondhugula added inline comments.
mlir/include/mlir/IR/PatternMatch.h
285

Sorry, I didn't understand. What does dialect conversion need from eraseBlock? and which it doesn't from eraseOp?

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1577

Thanks.

1580

Thanks, but I tend to prefer the way it is - I find it much simpler to read given the single line action.

mlir/lib/IR/PatternMatch.cpp
94

Sorry, I didn't understand - what's a non-trivially empty block? AFAICT, the block is either empty of ops or it isn't.

96

Sure.

bondhugula marked 3 inline comments as done.

Address review comments.

bondhugula marked an inline comment as done.Mar 30 2020, 1:31 PM
bondhugula added inline comments.
mlir/lib/IR/PatternMatch.cpp
94

It looks like I was looking at the wrong line / code context - fixed, thanks.

rriddle requested changes to this revision.Mar 30 2020, 1:33 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
285

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.

mlir/lib/IR/PatternMatch.cpp
94

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?

This revision now requires changes to proceed.Mar 30 2020, 1:33 PM

fix eraseBlock iteration order

bondhugula marked an inline comment as done.Mar 30 2020, 1:39 PM
bondhugula marked an inline comment as done.Mar 30 2020, 1:41 PM
bondhugula added inline comments.
mlir/include/mlir/IR/PatternMatch.h
285

Sure - but could you provide a pointer on what handling you need here? Is it trivial? It looks like one shouldn't use eraseBlock in such cases, but rely on a lower level approach?

Harbormaster completed remote builds in B51017: Diff 253687.
bondhugula edited reviewers, added: ftynse; removed: jpienaar.Apr 2 2020, 1:25 PM
bondhugula marked an inline comment as done and an inline comment as not done.
rriddle accepted this revision.Apr 2 2020, 6:08 PM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
285

I'm fine with adding a llvm_unreachable in the ConversionPatternRewriter for now.

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1577

I think you could just do: *ifOp.getElseBlock()

This revision is now accepted and ready to land.Apr 2 2020, 6:08 PM
bondhugula marked 6 inline comments as done.

Address review comments and rebase

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1577

Thanks.

This revision was automatically updated to reflect the committed changes.