[Mlir-commits] [mlir] afc800b - [mlir][transforms][NFC] Expand CanonicalizerPass documentation

[Matthias Springer]

Author: Matthias Springer
Date: 2023-01-03T12:39:12+01:00
New Revision: afc800b190a5ba080eb2aaf567e4a52d0c7eae82

URL: https://github.com/llvm/llvm-project/commit/afc800b190a5ba080eb2aaf567e4a52d0c7eae82
DIFF: https://github.com/llvm/llvm-project/commit/afc800b190a5ba080eb2aaf567e4a52d0c7eae82.diff

LOG: [mlir][transforms][NFC] Expand CanonicalizerPass documentation

Mention that canonicalization is best-effort and that pass pipelines should not rely on it for correctness.

RFC: https://discourse.llvm.org/t/rfc-canonicalizerpass-convergence-error-handling/67333

Differential Revision: https://reviews.llvm.org/D140729

Added: 
    

Modified: 
    mlir/docs/Canonicalization.md
    mlir/include/mlir/Transforms/Passes.td
    mlir/lib/Transforms/Canonicalizer.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/docs/Canonicalization.md b/mlir/docs/Canonicalization.md
index 1d3f053f32936..d1aed547eff9d 100644
--- a/mlir/docs/Canonicalization.md
+++ b/mlir/docs/Canonicalization.md
@@ -19,15 +19,28 @@ to capture IR-specific rules for reference.
 
 ## General Design
 
-MLIR has a single canonicalization pass, which iteratively applies
-canonicalization transformations in a greedy way until the IR converges. These
-transformations are defined by the operations themselves, which allows each
-dialect to define its own set of operations and canonicalizations together.
+MLIR has a single canonicalization pass, which iteratively applies the
+canonicalization patterns of all loaded dialects in a greedy way.
+Canonicalization is best-effort and not guaranteed to bring the entire IR in a
+canonical form. It applies patterns until either fixpoint is reached or the
+maximum number of iterations/rewrites (as specified via pass options) is
+exhausted. This is for efficiency reasons and to ensure that faulty patterns
+cannot cause infinite looping.
+
+Canonicalization patterns are registered with the operations themselves, which
+allows each dialect to define its own set of operations and canonicalizations
+together.
 
 Some important things to think about w.r.t. canonicalization patterns:
 
+*   Pass pipelines should not rely on the canonicalizer pass for correctness.
+    They should work correctly with all instances of the canonicalization pass
+    removed.
+
 *   Repeated applications of patterns should converge. Unstable or cyclic
-    rewrites will cause infinite loops in the canonicalizer.
+    rewrites are considered a bug: they can make the canonicalizer pass less
+    predictable and less effective (i.e., some patterns may not be applied) and
+    prevent it from converging.
 
 *   It is generally better to canonicalize towards operations that have fewer
     uses of a value when the operands are duplicated, because some patterns only

diff  --git a/mlir/include/mlir/Transforms/Passes.td b/mlir/include/mlir/Transforms/Passes.td
index e7d122323ae33..fd6351d90908d 100644
--- a/mlir/include/mlir/Transforms/Passes.td
+++ b/mlir/include/mlir/Transforms/Passes.td
@@ -20,7 +20,11 @@ def Canonicalizer : Pass<"canonicalize"> {
   let summary = "Canonicalize operations";
   let description = [{
     This pass performs various types of canonicalizations over a set of
-    operations. See [Operation Canonicalization](Canonicalization.md) for more
+    operations by iteratively applying the canonicalization patterns of all
+    loaded dialects until either a fixpoint is reached or the maximum number of
+    iterations/rewrites is exhausted. Canonicalization is best-effort and does
+    not guarantee that the entire IR is in a canonical form after running this
+    pass. See [Operation Canonicalization](Canonicalization.md) for more
     details.
   }];
   let constructor = "mlir::createCanonicalizerPass()";

diff  --git a/mlir/lib/Transforms/Canonicalizer.cpp b/mlir/lib/Transforms/Canonicalizer.cpp
index dc3bf97b32388..a6fa7750e36c0 100644
--- a/mlir/lib/Transforms/Canonicalizer.cpp
+++ b/mlir/lib/Transforms/Canonicalizer.cpp
@@ -57,6 +57,7 @@ struct Canonicalizer : public impl::CanonicalizerBase<Canonicalizer> {
     config.enableRegionSimplification = enableRegionSimplification;
     config.maxIterations = maxIterations;
     config.maxNumRewrites = maxNumRewrites;
+    // Canonicalization is best-effort. Non-convergence is not a pass failure.
     (void)applyPatternsAndFoldGreedily(getOperation(), patterns, config);
   }