[mlir][Canonicalize] Fix command-line options

[rkayaith]

Summary

The canonicalize command-line options currently have no effect, as the pass is
reading the pass options in its constructor, before they're actually
initialized. This results in the default values of the options always being used.

The change here moves the initialization of the GreedyRewriteConfig out of the
constructor, so that it runs after the pass options have been parsed.

Fixes #55466

[rkayaith]

mlir/lib/Transforms/Canonicalizer.cpp

53–54

I'll remove the print here, but any thoughts on how/whether to add a test for this?

[rriddle]

mlir/lib/Transforms/Canonicalizer.cpp

53–54

We could conceptually add a test to test-canonicalize.mlir that disables region simplification and checks to see that the simplifications don't fire?

[rkayaith]

add test

[rkayaith]

I'll need some help landing this once it looks good

mlir/lib/Transforms/Canonicalizer.cpp

 #include "mlir/Transforms/GreedyPatternRewriteDriver.h"
 #include "mlir/Transforms/Passes.h"
 
 using namespace mlir;
 
 namespace {
 /// Canonicalize operations in nested regions.
 struct Canonicalizer : public CanonicalizerBase<Canonicalizer> {
+  Canonicalizer() = default;
   Canonicalizer(const GreedyRewriteConfig &config,
                 ArrayRef<std::string> disabledPatterns,
-                ArrayRef<std::string> enabledPatterns)
-      : config(config) {
+                ArrayRef<std::string> enabledPatterns) {
+    this->topDownProcessingEnabled = config.useTopDownTraversal;
+    this->enableRegionSimplification = config.enableRegionSimplification;
+    this->maxIterations = config.maxIterations;
     this->disabledPatterns = disabledPatterns;
     this->enabledPatterns = enabledPatterns;
   }
 
-  Canonicalizer() {
-    // Default constructed Canonicalizer takes its settings from command line
-    // options.
-    config.useTopDownTraversal = topDownProcessingEnabled;
-    config.enableRegionSimplification = enableRegionSimplification;
-    config.maxIterations = maxIterations;
-  }
-
   /// Initialize the canonicalizer by building the set of patterns used during
   /// execution.
   LogicalResult initialize(MLIRContext *context) override {
     RewritePatternSet owningPatterns(context);
     for (auto *dialect : context->getLoadedDialects())
       dialect->getCanonicalizationPatterns(owningPatterns);
     for (RegisteredOperationName op : context->getRegisteredOperations())
       op.getCanonicalizationPatterns(owningPatterns, context);
 
     patterns = FrozenRewritePatternSet(std::move(owningPatterns),
                                        disabledPatterns, enabledPatterns);
     return success();
   }
   void runOnOperation() override {
-    (void)applyPatternsAndFoldGreedily(getOperation()->getRegions(), patterns,
-                                       config);
+    GreedyRewriteConfig config;
+    config.useTopDownTraversal = topDownProcessingEnabled;
+    config.enableRegionSimplification = enableRegionSimplification;
+    config.maxIterations = maxIterations;
+    (void)applyPatternsAndFoldGreedily(getOperation(), patterns, config);
   }
 
-  GreedyRewriteConfig config;
   FrozenRewritePatternSet patterns;
 };
 } // namespace
 
 /// Create a Canonicalizer pass.
 std::unique_ptr<Pass> mlir::createCanonicalizerPass() {
   return std::make_unique<Canonicalizer>();
 }

mlir/test/Transforms/test-canonicalize.mlir

 // RUN: mlir-opt %s -pass-pipeline='func.func(canonicalize)' | FileCheck %s
+// RUN: mlir-opt %s -pass-pipeline='func.func(canonicalize{region-simplify=false})' | FileCheck %s --check-prefixes=CHECK,NO-RS
 
 // CHECK-LABEL: func @remove_op_with_inner_ops_pattern
 func.func @remove_op_with_inner_ops_pattern() {
   // CHECK-NEXT: return
   "test.op_with_region_pattern"() ({
     "test.op_with_region_terminator"() : () -> ()
   }) : () -> ()
   return

 
 // CHECK-LABEL: test_dialect_canonicalizer
 func.func @test_dialect_canonicalizer() -> (i32) {
   %0 = "test.dialect_canonicalizable"() : () -> (i32)
   // CHECK: %[[CST:.*]] = arith.constant 42 : i32
   // CHECK: return %[[CST]]
   return %0 : i32
 }
+
+// Check that the option to control region simplification actually works
+// CHECK-LABEL: test_region_simplify
+func.func @test_region_simplify() {
+  // CHECK-NEXT:   return
+  // NO-RS-NEXT: ^bb1
+  // NO-RS-NEXT:   return
+  // CHECK-NEXT: }
+  return
+^bb1:
+  return
+}