[mlir][Transforms] GreedyPatternRewriteDriver: verify IR

[matthias-springer]

This commit adds an additional "expensive check" that verifies the IR before starting a greedy pattern rewriter, after every pattern application and after every folding. (Only if MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS is set.)

It also adds an assertion that the scope region (part of GreedyRewriteConfig) is not being erased as part of the greedy pattern rewrite. That would break the scoping mechanism and the expensive checks.

This commit does not fix any patterns, this is done in separate commits.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

This commit adds an additional "expensive check" that verifies the IR after every pattern application. (Only if MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS is set.)

There are currently 89 tests that are failing with this check. Many of them are in the vector and SparseTensor dialects, most of which are probably failing due to same reason. The actual number of patterns that produce invalid IR is probably in the order of 10.

This commit does not fix any patterns, this is done in separate commits.

---

Full diff: https://github.com/llvm/llvm-project/pull/74270.diff

1 Files Affected:

• (modified) mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp (+9)

diff --git a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
index 8e2bfe557c555..93a259a82cd2b 100644
--- a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
+++ b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
@@ -15,6 +15,7 @@
 #include "mlir/Config/mlir-config.h"
 #include "mlir/IR/Action.h"
 #include "mlir/IR/Matchers.h"
+#include "mlir/IR/Verifier.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
 #include "mlir/Rewrite/PatternApplicator.h"
 #include "mlir/Transforms/FoldUtils.h"
@@ -468,11 +469,19 @@ bool GreedyPatternRewriteDriver::processWorklist() {
         /*topLevel=*/config.scope ? config.scope->getParentOp() : op);
     auto clearFingerprints =
         llvm::make_scope_exit([&]() { debugFingerPrints.clear(); });
+    Operation *rootOp =
+        op->getParentWithTrait<mlir::OpTrait::IsIsolatedFromAbove>();
+    assert(rootOp && "expected non-null root op");
 #endif // MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS
 
     LogicalResult matchResult =
         matcher.matchAndRewrite(op, *this, canApply, onFailure, onSuccess);
 
+#if MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS
+    if (failed(verify(rootOp)))
+      llvm::report_fatal_error("IR failed to verify after pattern application");
+#endif // MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS
+
     if (succeeded(matchResult)) {
       LLVM_DEBUG(logResultWithLine("success", "pattern matched"));
 #if MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS

[matthias-springer]

Also see comment in #74271: Should we require patterns to generate valid IR? If all patterns are defined in the same component (e.g., canonicalization patterns of the same dialect) and the output is always valid, maybe intermediate invalid IR is OK. But things could get tricky when patterns from other components are mixed into the greedy pattern rewrite and stuff can suddenly break in unexpected ways.

[joker-eph]

Do we already have a bot checking with MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS?

[matthias-springer]

It could be added as an option next to MLIRContextImpl::allowUnregisteredDialects. But it would also require changes to MLIRContext and mlir-opt.cpp. I experimented with that in the beginning.

Another problem with this approach is that there is currently no API to signal a pass failure from the greedy pattern rewrite driver. I currently use llvm::report_fatal_error to make sure that mlir-opt stops executing. That's not really nice behavior for an mlir-opt option.

[Hardcode84]

I don't ask to change code immediately and I don't know what the best solution is, just some usability concerns.

Another problem with this approach is that there is currently no API to signal a pass failure from the greedy pattern rewrite driver.

applyPatternsAndFoldGreedily returns LogicalResult so it can be propagated as pass failure but I still see a lot of (void)applyPatternsAndFoldGreedily code in repo (mostly in test passes).

[joker-eph]

The return type of applyPatternsAndFoldGreedily is meant to signal failure to converge, which can be acceptable by the user.

[matthias-springer]

Do we already have a bot checking with MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS?

Not yet. One concern that I have is that some MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS checks can report false positives. The ones that look for "rewrite pattern returned success but IR did not change". The check is implemented by comparing the hash of the operations and there could be hash collisions. In practice, I haven't seen any hash collisions yet. Cloning the IR before every pattern application (to check for true equivalence) could be too expensive and may change the behavior of patterns (if Values get additional uses). What do you think?

[joker-eph]

I'm not worried about hash collision for this feature

[matthias-springer]

I have to fix 5 more tests (some non-trivial), then we can add the new bot:

Failed Tests (5):
  MLIR :: Conversion/VectorToSCF/vector-to-scf.mlir
  MLIR :: Dialect/ArmSME/tile-allocation-invalid.mlir
  MLIR :: Dialect/ArmSVE/legalize-vector-storage.mlir
  MLIR :: Dialect/Vector/vector-contract-to-outerproduct-transforms-unsupported.mlir
  MLIR :: Dialect/Vector/vector-warp-distribute.mlir