[mlir] Add eraseOp method to OpBuilder.

[mravishankar]

Summary

Depends On D90579

[rriddle]

I'm assuming the intent here is to allow for reusing the same bit of functionality from both pattern and non-pattern code? This is not the direction that I intended to move things given that I think it starts pushing OpBuilder further and further away from what it is intended to focus on(building IR). The direction I have intended to move to solve this problem is to evolve the PatternRewriter class into a more general OpRewriter class. The only method on PatternRewriter that is specific to patterns is notifyMatchFailure, but I think we could rename that to notifyRewriteFailure and it still works as a general concept. (At least, it looked fine in my WIP revision that does this, which I had intended to start pushing on soon). Thoughts on that direction?

mlir/include/mlir/IR/Builders.h

207

As an aside, a lot of work was done to make sure that OpBuilder does not have a vtable. Virtual hooks are added to the Listener class instead.

[mravishankar]

In D91100#2387845, @rriddle wrote:

I'm assuming the intent here is to allow for reusing the same bit of functionality from both pattern and non-pattern code? This is not the direction that I intended to move things given that I think it starts pushing OpBuilder further and further away from what it is intended to focus on(building IR). The direction I have intended to move to solve this problem is to evolve the PatternRewriter class into a more general OpRewriter class. The only method on PatternRewriter that is specific to patterns is notifyMatchFailure, but I think we could rename that to notifyRewriteFailure and it still works as a general concept. (At least, it looked fine in my WIP revision that does this, which I had intended to start pushing on soon). Thoughts on that direction?

Sorry I missed this comments (on my phab dashboard it only shows up as requires me to respond when "changes are requested"). The reason I added this was that there might be utility functions that are doing transformations but are not using the pattern rewrite mechanism. The changes dependent on this change are an example of that. To summarize, I have a sequence of operations that need to be "modified". It is actually not easy to write this as a pattern for multiple reasons. So I am writing this as just an IR transformation. In IREE we have encountered this before where we have a transformation that is not written as a pattern rewrite. So there we have had to work around cases where there is a necessary utility function in PatternRewriter but not OpBuilder.
I dont have any strong opinions on how to proceed here, but I need the utility method that is doing transformation to accept a builder, i.e. one of OpBuilder or PatternRewriter. I dont think I can instantiate a pattern rewriter outside of the the GreedyPatternRewriter right?

[mravishankar]

I refactored my dependent changes to not require this patch. So abandoning this for now. Would be good to resolve this though.

mlir/include/mlir/IR/Builders.h

explicit OpBuilder(Operation *op, Listener *listener = nullptr)
     setInsertionPoint(op);
   }
 
   OpBuilder(Block *block, Block::iterator insertPoint,
             Listener *listener = nullptr)
       : OpBuilder(block->getParent()->getContext(), listener) {
     setInsertionPoint(block, insertPoint);
   }
+  virtual ~OpBuilder() = default;
 
   /// Create a builder and set the insertion point to before the first operation
   /// in the block but still inside the block.
   static OpBuilder atBlockBegin(Block *block, Listener *listener = nullptr) {
     return OpBuilder(block, block->begin(), listener);
   }
 
   /// Create a builder and set the insertion point to after the last operation
public:
   }
   Operation *cloneWithoutRegions(Operation &op) {
     return insert(op.cloneWithoutRegions());
   }
   template <typename OpT> OpT cloneWithoutRegions(OpT op) {
     return cast<OpT>(cloneWithoutRegions(*op.getOperation()));
   }
 
+  /// This method erases an operation that is known to have no uses.
+  virtual void eraseOp(Operation *op);
+
 private:
   /// The current block this builder is inserting into.
   Block *block = nullptr;
   /// The insertion point within the block that this builder is inserting
   /// before.
   Block::iterator insertPoint;
   /// The optional listener for events of this builder.
   Listener *listener;
 };
 
 } // namespace mlir
 
 #endif

mlir/include/mlir/IR/PatternMatch.h

public:
   /// The result values of the two ops must be the same types.
   template <typename OpTy, typename... Args>
   void replaceOpWithNewOp(Operation *op, Args &&... args) {
     auto newOp = create<OpTy>(op->getLoc(), std::forward<Args>(args)...);
     replaceOpWithResultsOfAnotherOp(op, newOp.getOperation());
   }
 
   /// This method erases an operation that is known to have no uses.
-  virtual void eraseOp(Operation *op);
+  virtual void eraseOp(Operation *op) override;
 
   /// This method erases all operations in a block.
   virtual void eraseBlock(Block *block);
 
   /// Merge the operations of block 'source' into the end of block 'dest'.
   /// 'source's predecessors must either be empty or only contain 'dest`.
   /// 'argValues' is used to replace the block arguments of 'source' after
   /// merging.

mlir/lib/IR/Builders.cpp

Operation *OpBuilder::clone(Operation &op, BlockAndValueMapping &mapper) {
   }
   return insert(newOp);
 }
 
 Operation *OpBuilder::clone(Operation &op) {
   BlockAndValueMapping mapper;
   return clone(op, mapper);
 }
+
+void OpBuilder::eraseOp(Operation *op) {
+  assert(op->use_empty() && "expected 'op' to have no uses");
+  op->erase();
+}