[mlir] Remove ConversionPatternRewriter recursive optimization.

[tpopp]

Summary

Previously replaced ops would have all children with regions marked
as ignored also. This removes that change as it then erroneously ignored
regions that were later moved to other unignored/replaced regions.

This change might hurt performance of the ConversionPatternRewriter
by now checking ancestral ops to see if this op is "dangling" due to
any of those ops being removed, but should not be a driving performance
loss.

Note, this is not an NFC optimization removal because now some ops
that were being ignored due to a replace-and-move situation are not
ignored, and additionally, children of replaced ops were previously not
ignored in the optimization scheme but now will be if they are dangling.

[tpopp]

Let me know what you think of something like this River.

I was having issues where OpA containing OpB which contains OpC would not legalize OpC if OpA was changed and OpB was not.
Specifically, this code would result in inner ops being ignored because they were in the old op when Replace was called:

rewriter.replaceOp(assumingOp, newAssumingOp.getResults());
rewriter.inlineRegionBefore(assumingOp.doRegion(), newAssumingOp.doRegion(),
                            newAssumingOp.doRegion().end());

When looking at this more, I did not see any clean way to keep this optimization across different use cases of this method. It's hard to concisely explain. Let me know if you would like to discuss over a short video chat or similar.

[rriddle]

In D89877#2344423, @tpopp wrote:

Let me know what you think of something like this River.

I was having issues where OpA containing OpB which contains OpC would not legalize OpC if OpA was changed and OpB was not.
Specifically, this code would result in inner ops being ignored because they were in the old op when Replace was called:

rewriter.replaceOp(assumingOp, newAssumingOp.getResults());
rewriter.inlineRegionBefore(assumingOp.doRegion(), newAssumingOp.doRegion(),
                            newAssumingOp.doRegion().end());

This looks like an anti-pattern and only really works because of the internal implementation details of the conversion rewriter, i.e. if a pattern did this outside of the conversion infra it would segfault. replaceOp and eraseOp should be treated as the end of the lifetime for that operation and patterns shouldn't rely on the existence of the erased/replaced op(including its internals) after those calls. Given that, we don't intend to support these types of use cases and should ideally add some asserts to catch them early. Is this situation what you were running into or was it just similar?

When looking at this more, I did not see any clean way to keep this optimization across different use cases of this method. It's hard to concisely explain. Let me know if you would like to discuss over a short video chat or similar.

[tpopp]

Thank you for the explanation. My first example was the example, so I've reworked the code to move data before replacing an op. I will follow up with adding an assert to help prevent other people making the same mistake as me.

Also, just context on how I thought of this working in case it helps with any other decisions. When I thought of rewriter.replaceOp(Old, New), my assumption was that this replaced the uses of Old with New, but left the the op around until it was cleaned up later. With this model, I thought it was safe to continue using the Old op after replacing it.

[rriddle]

In D89877#2359475, @tpopp wrote:

Thank you for the explanation. My first example was the example, so I've reworked the code to move data before replacing an op. I will follow up with adding an assert to help prevent other people making the same mistake as me.

Also, just context on how I thought of this working in case it helps with any other decisions. When I thought of rewriter.replaceOp(Old, New), my assumption was that this replaced the uses of Old with New, but left the the op around until it was cleaned up later. With this model, I thought it was safe to continue using the Old op after replacing it.

Ah, yeah that would explain it. Do you see any gaps in documentation that could make this more clear? Either on the PatternRewriter class definition or in https://mlir.llvm.org/docs/PatternRewriter/#pattern-rewriter?

mlir/lib/Transforms/DialectConversion.cpp

if (converter && desiredType && newOperandType != desiredType) {
       }
     }
     remapped.push_back(newOperand);
   }
   return success();
 }
 
 bool ConversionPatternRewriterImpl::isOpIgnored(Operation *op) const {
-  // Check to see if this operation was replaced or its parent ignored.
-  return replacements.count(op) || ignoredOps.count(op->getParentOp());
+  // Check to see if this operation or a parent was replaced or ignored.
+  do {
+    if (replacements.count(op))
+      return true;
+    if (ignoredOps.count(op))
+      return true;
+  } while ((op = op->getParentOp()));
+  return false;
 }
 
 void ConversionPatternRewriterImpl::markNestedOpsIgnored(Operation *op) {
-  // Walk this operation and collect nested operations that define non-empty
-  // regions. We mark such operations as 'ignored' so that we know we don't have
-  // to convert them, or their nested ops.
-  if (op->getNumRegions() == 0)
-    return;
-  op->walk([&](Operation *op) {
-    if (llvm::any_of(op->getRegions(),
-                     [](Region &region) { return !region.empty(); }))
-      ignoredOps.insert(op);
-  });
+  // Mark operation as ignored and all children will be recursively considered
+  // ignored also.
 }
 
 //===----------------------------------------------------------------------===//
 // Type Conversion
 
 FailureOr<Block *> ConversionPatternRewriterImpl::convertBlockSignature(
     Block *block, TypeConverter &converter,
     TypeConverter::SignatureConversion *conversion) {
void ConversionPatternRewriterImpl::notifyOpReplaced(Operation *op,
   if (resultChanged)
     operationsWithChangedResults.push_back(replacements.size());
 
   // Record the requested operation replacement.
   TypeConverter *converter = nullptr;
   if (currentConversionPattern)
     converter = currentConversionPattern->getTypeConverter();
   replacements.insert(std::make_pair(op, OpReplacement(converter)));
-
-  // Mark this operation as recursively ignored so that we don't need to
-  // convert any nested operations.
-  markNestedOpsIgnored(op);
 }
 
 void ConversionPatternRewriterImpl::notifyBlockIsBeingErased(Block *block) {
   Region *region = block->getParent();
   Block *origPrevBlock = block->getPrevNode();
   blockActions.push_back(BlockAction::getErase(block, {region, origPrevBlock}));
 }