[MLIR] Fix dialect conversion cancelRootUpdate

[bondhugula]

Summary

Fix dialect conversion ConversionPatternRewriter::cancelRootUpdate: the
erasure of operations here from the list of root update was off by one.
Should have been:

rootUpdates.erase(rootUpdates.begin() + (rootUpdates.rend() - it - 1));

instead of

rootUpdates.erase(rootUpdates.begin() + (rootUpdates.rend() - it));

or more directly:

rootUpdates.erase(it.base() -1)

While on this, add an assertion to improve dev experience when a cancel is
called on an op on which a root update hasn't been started.

[bondhugula]

Update commit summary a bit.

[bondhugula]

Fix iterator and commit message.

[mehdi_amini]

mlir/lib/Transforms/Utils/DialectConversion.cpp

1488

This formula is confusing me. And std::prev(rootUpdates.rend()) even more so. How is it different from begin()?

[mehdi_amini]

mlir/lib/Transforms/Utils/DialectConversion.cpp

1488

Re-reading, it will be a reverse iterator, and you're trying to get its position here.
Maybe name a variable for this? int updateIdx = std::prev(rootUpdates.rend()) - it;

2053

Can you write const Pattern * instead of auto?

[bondhugula]

NFC. Putting position in a var.

[bondhugula]

mlir/lib/Transforms/Utils/DialectConversion.cpp

1488

Not sure how naming a variable here helps but I've done it. (Anything coming after begin() + will always be read as a position).

[bondhugula]

mlir/lib/Transforms/Utils/DialectConversion.cpp

2053

This won't be const Pattern *, but a const Pattern *const *. It's an iterator. I fixed this one since my in-editor checker (YCM) complains due to the clang-tidy check. This is annoying as it would mask other more non-borderline style errors. We can leave it at auto *it.

mlir/lib/Transforms/Utils/DialectConversion.cpp

 #ifndef NDEBUG
   assert(impl->pendingRootUpdates.erase(op) &&
          "operation did not have a pending in-place update");
 #endif
   // Erase the last update for this operation.
   auto stateHasOp = [op](const auto &it) { return it.getOperation() == op; };
   auto &rootUpdates = impl->rootUpdates;
   auto it = llvm::find_if(llvm::reverse(rootUpdates), stateHasOp);
-  rootUpdates.erase(rootUpdates.begin() + (rootUpdates.rend() - it));
+  assert(it != rootUpdates.rend() && "no root update started on op");
+  rootUpdates.erase(it.base());
 }
 
 /// PatternRewriter hook for notifying match failure reasons.
 LogicalResult ConversionPatternRewriter::notifyMatchFailure(
     Operation *op, function_ref<void(Diagnostic &)> reasonCallback) {
   return impl->notifyMatchFailure(op->getLoc(), reasonCallback);
 }
 
void OperationLegalizer::computeLegalizationGraphBenefit(
   applicator.applyCostModel([&](const Pattern &pattern) {
     ArrayRef<const Pattern *> orderedPatternList;
     if (Optional<OperationName> rootName = pattern.getRootKind())
       orderedPatternList = legalizerPatterns[*rootName];
     else
       orderedPatternList = anyOpLegalizerPatterns;
 
     // If the pattern is not found, then it was removed and cannot be matched.
-    auto it = llvm::find(orderedPatternList, &pattern);
+    auto *it = llvm::find(orderedPatternList, &pattern);
     if (it == orderedPatternList.end())
       return PatternBenefit::impossibleToMatch();
 
     // Patterns found earlier in the list have higher benefit.
     return PatternBenefit(std::distance(it, orderedPatternList.end()));
   });
 }