[mlir] GreedyPatternRewriter: Reprocess modified ops

[springerm]

Summary

Ops that were modifed in-place (finalizeRootUpdate was called) should be reprocessed by the GreedyPatternRewriter. This is currently not happening with GreedyRewriteConfig::maxIterations = 1.

Note: If your project goes into an infinite loop because of this change, you likely have one or multiple faulty patterns that modify the same operations in-place (updateRootInplace) indefinitely.

[springerm]

(There is still a problem with this change. It is crashing vector-transfer-full-partial-split.mlir.)

[mehdi_amini]

Is this something we can test for?

[rriddle]

The change makes sense to me, but it'd be nice to add a test if we can.

[springerm]

address comments

[springerm]

typo

[rriddle]

mlir/unittests/Transforms/GreedyPatternRewriteDriver.cpp

19–31 ↗

(On Diff #475732)

It would be nice if we could just have something on one of the existing test passes, instead of using a unittest. We generally don't use unittests to test IR/pattern/transformation/etc. changes.

See mlir/test/lib/Dialect/Test/TestPatterns.cpp

[springerm]

address comments

[springerm]

update

[rriddle]

LGTM, but the windows build failure there looks real. Please resolve that before landing.

[springerm]

fix Windows build

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

public:
   virtual void addToWorklist(Operation *op);
 
   /// Pop the next operation from the worklist.
   Operation *popFromWorklist();
 
   /// If the specified operation is in the worklist, remove it.
   void removeFromWorklist(Operation *op);
 
+  /// Notifies the driver that the specified operation may have been modified
+  /// in-place.
+  void finalizeRootUpdate(Operation *op) override;
+
 protected:
   // Implement the hook for inserting operations, and make sure that newly
   // inserted ops are added to the worklist for processing.
   void notifyOperationInserted(Operation *op) override;
 
   // Look over the provided operands for any defining operations that should
   // be re-added to the worklist. This function should be called when an
   // operation is modified or removed, as it may trigger further

 void GreedyPatternRewriteDriver::notifyOperationInserted(Operation *op) {
   LLVM_DEBUG({
     logger.startLine() << "** Insert  : '" << op->getName() << "'(" << op
                        << ")\n";
   });
   addToWorklist(op);
 }
 
+void GreedyPatternRewriteDriver::finalizeRootUpdate(Operation *op) {
+  LLVM_DEBUG({
+    logger.startLine() << "** Modified: '" << op->getName() << "'(" << op
+                       << ")\n";
+  });
+  addToWorklist(op);
+}
+
 void GreedyPatternRewriteDriver::addOperandsToWorklist(ValueRange operands) {
   for (Value operand : operands) {
     // If the use count of this operand is now < 2, we re-add the defining
     // operation to the worklist.
     // TODO: This is based on the fact that zero use operations
     // may be deleted, and that single use values often have more
     // canonicalization opportunities.
     if (!operand || (!operand.use_empty() && !operand.hasOneUse()))