[ODS] Implement a new 'hasCanonicalizeMethod' bit for cann patterns.

[lattner]

Summary

This provides a simplified way to implement 'matchAndRewrite' style
canonicalization patterns for ops that don't need the full power of
RewritePatterns. Using this style, you can implement a static method
with a signature like:

LogicalResult AssertOp::canonicalize(AssertOp op, PatternRewriter &rewriter) {
  return success();
}

instead of dealing with defining RewritePattern subclasses.

This also adopts this for a few patterns in the standardops dialect
to show how it works.

[lattner]

Change commit message.

[lattner]

update commit message in phab.

[jpienaar]

I'd prefer something like

let canonicalizer = [Pat<(...)>, ...];

Then you can use DRR (currently tablegen dag format, potentially other frontend post) to write these. The simple one could be expressed as

Pat<(Op:$op _,_,_), (NativeCodeCall<"canonicalizer"> $_builder, $_self)>

Or we could introduce a shorthand for it (potentially start with not needing to have match side of pattern for this pattern and then one can get this as tablegen def without modifying driver post - that same first step also allows generating the function pointer variant ones if we prefered for these). So you'd get

let canonicalizer = [CanonicalizeOnOp]

but could expand to writing other patterns adjacent to this & one could then also shared common patterns between multiple of these ops more declaratively. And it allows a single specification, so you can add all your patterns together and the C++ add method could be generated for you (so that hasCanonicalizer need not be treated as orthogonal but as something that can be removed/subsumed)

[lattner]

I'm not sure I understand the ask here - that sounds like a separate feature. We already have a way to specify canonicalization patterns with ODS, this is intended to help the C++ case.

A change to: "Pat<(Op:$op _,_,_), (NativeCodeCall<"canonicalizer"> $_builder, $_self)>" moves the boilerplate around and makes it more obscure, it doesn't eliminate it.

[mehdi_amini]

I love the reduced boilerplate! :)
But I'd like to understand more what Jacques has in mind, I didn't quite get the comment either.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1706

What happens if both hasCanonicalizer and hasCanonicalizeMethod are set?
I have the impression that the code isn't defensive here, or that hasCanonicalizer would be ignored in this case? If this is the case I rather error out telling the user that they are exclusive.

[lattner]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1706

If both are set then you get a prototype for getCanonicalizationPatterns and for canonicalize in the header, but they you implement both of them however you'd like.

[mehdi_amini]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1678

Nit, it'd read better to me as:

bool hasCanonicalizeMethod = def.getValueAsBit("hasCanonicalizeMethod");
if (hasCanonicalizeMethod) {
   ...

(making it more clear that hasCanonicalizeMethod value is directly the Tablegen value for the flag) instead of assigning in the body of the conditional)

1706

Mmmmm, reading the code I see getCanonicalizationPatterns getting a generated body just based on hasCanonicalizeMethod regardless of hasCanonicalizer.
From what you're answering me here I'd have expected to read:

if (hasCanonicalizeMethod && !def.getValueAsBit("hasCanonicalizer"))
  method->body() << "  results.add(canonicalize);\n";

[rriddle]

Looks good!

Crazy half thought out idea:

Thinking about the feature that Jacques is talking about and the one being added here, I wonder if we should adopt a different approach to the current normal case for canonicalization patterns. By that I mean, should we rename the current getCanonicalizationPatterns that users implement to something else? That would allow for ODS to generate the external facing getCanonicalizationPatterns and automatically add all of the different ways to add patterns, e.g. something like:

class MyOp {
  /// The external facing call.
  void getCanonicalizationPatterns(RewritePatternSet &patterns) {
    // Add the native patterns.
    getCanonicalizationPatternsImpl(patterns);
    getCanonicalizationPatterns.add(canonicalize);
    
    // Add any DRR patterns defined on the op in ODS.
    getDRRCanonicalizationPatterns(patterns);
  }

private:
  /// The things that the author of `MyOp` implements.
  void getCanonicalizationPatternsImpl(RewritePatternSet &patterns);
  LogicalResult canonicalize(MyOp op, PatternRewriter &rewriter);

  /// Code generate by ODS to add canonicalization patterns defined in DRR.
  void getDRRCanonicalizationPatterns(RewritePatternSet &patterns);
};

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

311–315

This is really nice!

528–532

Why merge these? From the discussion on the forums, it seems like there is still some contention on merging multiple patterns into one (unless this was resolved?). Wouldn't it be possible to still have this implemented as(with the same amount of code):

void BranchOp::getCanonicalizationPatterns(RewritePatternSet &results,
                                           MLIRContext *context) {
  results.add(simplifyBrToBlockWithSinglePred, simplifyPassThroughBr);
}

(Though that wouldn't exercise the feature you are added in this patch)

[lattner]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1678

Good point!

1706

You're right, I simplified the code to make it more obvious what is going on here, thank you for pointing this out!

[lattner]

Clarify the code in tblgen, thanks to Mehdi's feedback

[lattner]

Crazy half thought out idea:

That is not a crazy idea. I think it is beautiful.

This step also makes be grumpy about the existing functionality like this:

let printer = [{ return ::print(p, *this); }];
let verifier = [{ return ::verify(*this); }];
let parser = [{ return ::parse$cppClass(parser, result); }];

These have annoyed me for some time for a couple of reasons:

1. it leads to unnecessary "innovation" in implementation approaches for these hooks.
2. It leads to unnecessary c++ code in tblgen, and encourages people to put stuff inline.
3. One of the standard approaches (shown above) depends on overloading of "::verify" and other methods, which leads to terrible error messages from clang when you mess something up, because the compiler spews the full overload set.

I think that going to a model of:

let hasCustomPrinter = 1;
let hasCustomVerifier = 1;
let hasCustomParser = 1;

would make sense, and then you implement a "well known method", like folder and now canonicalizer do.

With such an approach, your suggestion would fit naturally in line:

let hasCustomCanonicalizerPatterns = 1;

or something.

[lattner]

That said, I think that is clearly out of bounds for this patch ;-). Does anyone object to landing this? It seems consistent with the logical path forward here.

[rriddle]

LGTM as an initial patch (assuming all comments have been resolved)

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

528–532

(Missed?)

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1684

[mehdi_amini]

In D99143#2645571, @lattner wrote:

This step also makes be grumpy about the existing functionality like this:

let printer = [{ return ::print(p, *this); }];
let verifier = [{ return ::verify(*this); }];
let parser = [{ return ::parse$cppClass(parser, result); }];

These have annoyed me for some time for a couple of reasons:

1. it leads to unnecessary "innovation" in implementation approaches for these hooks.
2. It leads to unnecessary c++ code in tblgen, and encourages people to put stuff inline.
3. One of the standard approaches (shown above) depends on overloading of "::verify" and other methods, which leads to terrible error messages from clang when you mess something up, because the compiler spews the full overload set.

I think that going to a model of:

let hasCustomPrinter = 1;
let hasCustomVerifier = 1;
let hasCustomParser = 1;

Oh yes please!!! :)

[lattner]

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

528–532

Yep I missed this, thanks for the ping.

Sure, I could do this, but how in any way is this _better_? This would add more codesize bloat, and more patterns for the pattern rewriter to juggle. Merging into a single function is functionally identical and more efficient.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1684

Good catch, thanks!

[lattner]

namespace qualify LogicalResult as River pointed out.

[rriddle]

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

528–532

For very simple transformations like this specific instance, it doesn't really matter (at least IMO). In generality though, having directed patterns makes it much easier to debug and reason about what is going on. We have facilities for logging which patterns are executing, controlling their execution(ala debug counters, with ideas for more sophisticated systems being devised; such as enabling/disabling specific patterns), and more. Creating mega-patterns makes the system that much more difficult to interact with (from my experience).

As to the performance related concerns, having looked deeply into where the code size in MLIR is(I heavily optimized quite a few components a while ago to better enable running MLIR on-device), the impact from the pattern wrapper classes was negligible in the grand scheme of things. For the second point, the pattern rewriter already performs op-specific bucketing, and splitting into two patterns isn't that dramatic of a change in execution time (it drops an iteration from the matcher loop for this specific operation type, and removes an indirect call).

Anyways, none of that blocks this specific patch (IMO) but it does bring about a need to document general guidelines on when should a pattern be split into multiple vs mega pattern.

[lattner]

Got it, thanks. I committed this patch and will follow up with an enhancement to allow multiple function pointers to be added as individual patterns all at one time.

[lattner]

Actually, I don't think we need to do anything here. We already support patterns.add(foo).add(bar).add(baz); we could support patterns.add(foo, bar, baz); through more application of variadic templates, but I don't see a super important reason to do so. We can add it if there is a reason to in the future.

[rriddle]

In D99143#2645939, @lattner wrote:

Actually, I don't think we need to do anything here. We already support patterns.add(foo).add(bar).add(baz); we could support patterns.add(foo, bar, baz); through more application of variadic templates, but I don't see a super important reason to do so. We can add it if there is a reason to in the future.

SGTM, completely forgot that we have chaining. Thanks!

mlir/docs/Canonicalization.md

 *   `constant-like` operations are uniqued and hoisted into the entry block of
     the first parent barrier region. This is a region that is either isolated
     from above, e.g. the entry block of a function, or one marked as a barrier
     via the `shouldMaterializeInto` method on the `DialectFoldInterface`.
 
 ## Defining Canonicalizations
 
 Two mechanisms are available with which to define canonicalizations;
-`getCanonicalizationPatterns` and `fold`.
+general `RewritePattern`s and the `fold` method.
 
-### Canonicalizing with `getCanonicalizationPatterns`
+### Canonicalizing with `RewritePattern`s
 
 This mechanism allows for providing canonicalizations as a set of
 `RewritePattern`s, either imperatively defined in C++ or declaratively as
 [Declarative Rewrite Rules](DeclarativeRewrites.md). The pattern rewrite
 infrastructure allows for expressing many different types of canonicalizations.
 These transformations may be as simple as replacing a multiplication with a
 shift, or even replacing a conditional branch with an unconditional one.
 
-In [ODS](OpDefinitions.md), an operation can set the `hasCanonicalizer` bit to
-generate a declaration for the `getCanonicalizationPatterns` method.
+In [ODS](OpDefinitions.md), an operation can set the `hasCanonicalizer` bit or
+the `hasCanonicalizeMethod` bit to generate a declaration for the
+`getCanonicalizationPatterns` method:
 
 ```tablegen
 def MyOp : ... {
+  // I want to define a fully general set of patterns for this op.
   let hasCanonicalizer = 1;
 }
+
+def OtherOp : ... {
+  // A single "matchAndRewrite" style RewritePattern implemented as a method
+  // is good enough for me.
+  let hasCanonicalizeMethod = 1;
+}
 ```
 
 Canonicalization patterns can then be provided in the source file:
 
 ```c++
 void MyOp::getCanonicalizationPatterns(RewritePatternSet &patterns,
                                        MLIRContext *context) {
   patterns.add<...>(...);
 }
+
+LogicalResult OtherOp::canonicalize(OtherOp op, PatternRewriter &rewriter) {
+  // patterns and rewrites go here.
+  return failure();
+}
 ```
 
 See the [quickstart guide](Tutorials/QuickstartRewrites.md) for information on
 defining operation rewrites.
 
-### Canonicalizing with `fold`
+### Canonicalizing with the `fold` method
 
 The `fold` mechanism is an intentionally limited, but powerful mechanism that
 allows for applying canonicalizations in many places throughout the compiler.
 For example, outside of the canonicalizer pass, `fold` is used within the
 [dialect conversion infrastructure](#DialectConversion.md) as a legalization
 mechanism, and can be invoked directly anywhere with an `OpBuilder` via
 `OpBuilder::createOrFold`.
 

mlir/docs/OpDefinitions.md

 `SameOperandsAndResultType`.
 
 ### `hasCanonicalizer`
 
 This boolean field indicate whether canonicalization patterns have been defined
 for this operation. If it is `1`, then `::getCanonicalizationPatterns()` should
 be defined.
 
+### `hasCanonicalizeMethod`
+
+When this boolean field is set to `true`, it indicates that the op implements a
+`canonicalize` method for simple "matchAndRewrite" style canonicalization
+patterns.  If `hasCanonicalizer` is 0, then an implementation of
+`::getCanonicalizationPatterns()` is implemented to call this function.
+
 ### `hasFolder`
 
 This boolean field indicate whether general folding rules have been defined for
 this operation. If it is `1`, then `::fold()` should be defined.
 
 ### Extra declarations
 
 One of the goals of table-driven op definition is to auto-generate as much logic

mlir/docs/Tutorials/QuickstartRewrites.md

 
 Then you can `#include` the generated file in any C++ implementation file you
 like. (You will also need to make sure the library depends on the CMake target
 defined in the above.) The generated file will have a `populateWithGenerated(
 RewritePatternSet &patterns)` function that you can
 use to collect all the generated patterns inside `patterns` and then use
 `patterns` in any pass you would like.
 
-### C++ rewrite specification
+### Simple C++ `matchAndRewrite` style specifications
 
-In case patterns are not sufficient there is also the fully C++ way of
-expressing a rewrite:
+Many simple rewrites can be expressed with a `matchAndRewrite` style  of
+pattern, e.g. when converting a multiply by a power of two into a shift.  For
+these cases, the you can define the pattern as a simple function:
+
+```c++
+static LogicalResult
+convertTFLeakyRelu(TFLeakyReluOp op, PatternRewriter &rewriter) {
+  rewriter.replaceOpWithNewOp<TFL::LeakyReluOp>(
+      op, op->getResult(0).getType(), op->getOperand(0),
+      /*alpha=*/op->getAttrOfType<FloatAttr>("alpha"));
+  return success();
+}
+
+void populateRewrites(RewritePatternSet &patternSet) {
+  // Add it to a pattern set.
+  patternSet.add(convertTFLeakyRelu);
+}
+```
+
+ODS provides a simple way to define a function-style canonicalization for your
+operation.  In the TableGen definition of the op, specify
+`let hasCanonicalizeMethod = 1;` and then implement the `canonicalize` method in
+your .cpp file:
+
+```c++
+// Example from the CIRCT project which has a variadic integer multiply.
+LogicalResult circt::MulOp::canonicalize(MulOp op, PatternRewriter &rewriter) {
+  auto inputs = op.inputs();
+  APInt value;
+
+  // mul(x, c) -> shl(x, log2(c)), where c is a power of two.
+  if (inputs.size() == 2 && matchPattern(inputs.back(), m_RConstant(value)) &&
+      value.isPowerOf2()) {
+    auto shift = rewriter.create<rtl::ConstantOp>(op.getLoc(), op.getType(),
+                                                  value.exactLogBase2());
+    auto shlOp =
+        rewriter.create<comb::ShlOp>(op.getLoc(), inputs[0], shift);
+    rewriter.replaceOpWithNewOp<MulOp>(op, op.getType(),
+                                       ArrayRef<Value>(shlOp));
+    return success();
+  }
+
+  return failure();
+}
+```
+
+However, you may want the full generality of canonicalization patterns, for that
+you can specify an arbitrary list of `RewritePattern`s.
+
+### Fully general C++ `RewritePattern` specifications
+
+In case ODS patterns and `matchAndRewrite`-style functions are not sufficient
+you can also specify rewrites as a general set of `RewritePattern`s:
 
 ```c++
 /// Multi-step rewrite using "match" and "rewrite". This allows for separating
 /// the concerns of matching and rewriting.
 struct ConvertTFLeakyRelu : public RewritePattern {
   ConvertTFLeakyRelu(MLIRContext *context)
       : RewritePattern("tf.LeakyRelu", 1, context) {}
 
struct ConvertTFLeakyRelu : public RewritePattern {
   }
 };
 ```
 
 In the C++ rewrite the static benefit of the rewrite pattern is specified at
 construction. While in the pattern generator a simple heuristic is currently
 employed based around the number of ops matched and replaced.
 
-In the case where you have a registered op and want to use a benefit of 1, you
-can even define the pattern as a C function:
-
-```c++
-static LogicalResult
-convertTFLeakyRelu(TFLeakyReluOp op, PatternRewriter &rewriter) {
-  rewriter.replaceOpWithNewOp<TFL::LeakyReluOp>(
-      op, op->getResult(0).getType(), op->getOperand(0),
-      /*alpha=*/op->getAttrOfType<FloatAttr>("alpha"));
-  return success();
-}
-```
-
 The above rule did not capture the matching operands/attributes, but in general
 the `match` function in a multi-step rewrite may populate and return a
 `PatternState` (or class derived from one) to pass information extracted during
 matching to the rewrite. A single-step rewrite with the `matchAndRewrite`
 function has the benefit of being able to directly use any values created when
 matching; removing the need for `PatternState`.
 
 ## Testing

mlir/include/mlir/Dialect/StandardOps/IR/Ops.h

 // Pull in all enum type definitions and utility function declarations.
 #include "mlir/Dialect/StandardOps/IR/OpsEnums.h.inc"
 
 namespace mlir {
 class AffineMap;
 class Builder;
 class FuncOp;
 class OpBuilder;
+class PatternRewriter;
 
 /// Return the list of Range (i.e. offset, size, stride). Each Range
 /// entry contains either the dynamic value or a ConstantIndexOp constructed
 /// with `b` at location `loc`.
 SmallVector<Range, 8> getOrCreateRanges(OffsetSizeAndStrideOpInterface op,
                                         OpBuilder &b, Location loc);
 
 #define GET_OP_CLASSES

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td

def AssertOp : Std_Op<"assert"> {
 
   let arguments = (ins I1:$arg, StrAttr:$msg);
 
   let assemblyFormat = "$arg `,` $msg attr-dict";
 
   // AssertOp is fully verified by its traits.
   let verifier = ?;
 
-  let hasCanonicalizer = 1;
+  let hasCanonicalizeMethod = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // AtomicRMWOp
 //===----------------------------------------------------------------------===//
 
 def AtomicRMWOp : Std_Op<"atomic_rmw", [
       AllTypesMatch<["value", "result"]>,
def BranchOp : Std_Op<"br",
   let extraClassDeclaration = [{
     Block *getDest();
     void setDest(Block *block);
 
     /// Erase the operand at 'index' from the operand list.
     void eraseOperand(unsigned index);
   }];
 
-  let hasCanonicalizer = 1;
+  let hasCanonicalizeMethod = 1;
   let assemblyFormat = [{
     $dest (`(` $destOperands^ `:` type($destOperands) `)`)? attr-dict
   }];
 }
 
 //===----------------------------------------------------------------------===//
 // CallOp
 //===----------------------------------------------------------------------===//
let extraClassDeclaration = [{
     operand_iterator arg_operand_begin() { return ++operand_begin(); }
     operand_iterator arg_operand_end() { return operand_end(); }
 
     /// Return the callee of this operation.
     CallInterfaceCallable getCallableForCallee() { return getCallee(); }
   }];
 
   let verifier = ?;
-  let hasCanonicalizer = 1;
+  let hasCanonicalizeMethod = 1;
 
   let assemblyFormat = "$callee `(` $operands `)` attr-dict `:` type($callee)";
 }
 
 //===----------------------------------------------------------------------===//
 // CeilFOp
 //===----------------------------------------------------------------------===//
 

mlir/include/mlir/IR/OpBase.td

class Op<Dialect dialect, string mnemonic, list<OpTrait> props = []> {
 
   // Custom assembly format.
   string assemblyFormat = ?;
 
   // Custom verifier.
   code verifier = ?;
 
   // Whether this op has associated canonicalization patterns.
-  // TODO: figure out a better way to write canonicalization patterns in
-  // TableGen rules directly instead of using this marker and C++
-  // implementations.
   bit hasCanonicalizer = 0;
 
+  // Whether this op has a static "canonicalize" method to perform "match and
+  // rewrite patterns".
+  bit hasCanonicalizeMethod = 0;
+
   // Whether this op has a folder.
   bit hasFolder = 0;
 
   // Op traits.
   // Note: The list of traits will be uniqued by ODS.
   list<OpTrait> traits = props;
 
   // Additional code that will be added to the public part of the generated

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

OpFoldResult AndOp::fold(ArrayRef<Attribute> operands) {
   return constFoldBinaryOp<IntegerAttr>(operands,
                                         [](APInt a, APInt b) { return a & b; });
 }
 
 //===----------------------------------------------------------------------===//
 // AssertOp
 //===----------------------------------------------------------------------===//
 
-namespace {
-struct EraseRedundantAssertions : public OpRewritePattern<AssertOp> {
-  using OpRewritePattern<AssertOp>::OpRewritePattern;
-
-  LogicalResult matchAndRewrite(AssertOp op,
-                                PatternRewriter &rewriter) const override {
-    // Erase assertion if argument is constant true.
-    if (matchPattern(op.arg(), m_One())) {
-      rewriter.eraseOp(op);
-      return success();
-    }
-    return failure();
-  }
-};
-} // namespace
-
-void AssertOp::getCanonicalizationPatterns(RewritePatternSet &patterns,
-                                           MLIRContext *context) {
-  patterns.add<EraseRedundantAssertions>(context);
-}
+LogicalResult AssertOp::canonicalize(AssertOp op, PatternRewriter &rewriter) {
 
 //===----------------------------------------------------------------------===//
 // AtomicRMWOp
 //===----------------------------------------------------------------------===//
 
 static LogicalResult verify(AtomicRMWOp op) {
   if (op.getMemRefType().getRank() != op.getNumOperands() - 2)
     return op.emitOpError(
for (Value operand : operands) {
     else
       argStorage.push_back(operand);
   }
   successor = successorDest;
   successorOperands = argStorage;
   return success();
 }
 
-namespace {
 /// Simplify a branch to a block that has a single predecessor. This effectively
 /// merges the two blocks.
-struct SimplifyBrToBlockWithSinglePred : public OpRewritePattern<BranchOp> {
-  using OpRewritePattern<BranchOp>::OpRewritePattern;
-
-  LogicalResult matchAndRewrite(BranchOp op,
-                                PatternRewriter &rewriter) const override {
-    // Check that the successor block has a single predecessor.
-    Block *succ = op.getDest();
-    Block *opParent = op->getBlock();
-    if (succ == opParent || !llvm::hasSingleElement(succ->getPredecessors()))
-      return failure();
+static LogicalResult
+simplifyBrToBlockWithSinglePred(BranchOp op, PatternRewriter &rewriter) {
 
-    // Merge the successor into the current block and erase the branch.
-    rewriter.mergeBlocks(succ, opParent, op.getOperands());
-    rewriter.eraseOp(op);
-    return success();
-  }
-};
 
 ///   br ^bb1
 /// ^bb1
 ///   br ^bbN(...)
 ///
 ///  -> br ^bbN(...)
 ///
-struct SimplifyPassThroughBr : public OpRewritePattern<BranchOp> {
-  using OpRewritePattern<BranchOp>::OpRewritePattern;
-
-  LogicalResult matchAndRewrite(BranchOp op,
-                                PatternRewriter &rewriter) const override {
-    Block *dest = op.getDest();
-    ValueRange destOperands = op.getOperands();
-    SmallVector<Value, 4> destOperandStorage;
+static LogicalResult simplifyPassThroughBr(BranchOp op,
+                                           PatternRewriter &rewriter) {
 
-    // Try to collapse the successor if it points somewhere other than this
-    // block.
-    if (dest == op->getBlock() ||
-        failed(collapseBranch(dest, destOperands, destOperandStorage)))
-      return failure();
 
-    // Create a new branch with the collapsed successor.
-    rewriter.replaceOpWithNewOp<BranchOp>(op, dest, destOperands);
-    return success();
-  }
-};
-} // end anonymous namespace.
+
+LogicalResult BranchOp::canonicalize(BranchOp op, PatternRewriter &rewriter) {
+  return success(succeeded(simplifyBrToBlockWithSinglePred(op, rewriter)) ||
+                 succeeded(simplifyPassThroughBr(op, rewriter)));
+}
 
 Block *BranchOp::getDest() { return getSuccessor(); }
 
 void BranchOp::setDest(Block *block) { return setSuccessor(block); }
 
 void BranchOp::eraseOperand(unsigned index) { (*this)->eraseOperand(index); }
 
-void BranchOp::getCanonicalizationPatterns(RewritePatternSet &results,
-                                           MLIRContext *context) {
-  results.add<SimplifyBrToBlockWithSinglePred, SimplifyPassThroughBr>(context);
-}
-
 Optional<MutableOperandRange>
 BranchOp::getMutableSuccessorOperands(unsigned index) {
   assert(index == 0 && "invalid successor index");
   return destOperandsMutable();
 }
 
 Block *BranchOp::getSuccessorForOperands(ArrayRef<Attribute>) { return dest(); }
 

 
 FunctionType CallOp::getCalleeType() {
   return FunctionType::get(getContext(), getOperandTypes(), getResultTypes());
 }
 
 //===----------------------------------------------------------------------===//
 // CallIndirectOp
 //===----------------------------------------------------------------------===//
-namespace {
-/// Fold indirect calls that have a constant function as the callee operand.
-struct SimplifyIndirectCallWithKnownCallee
-    : public OpRewritePattern<CallIndirectOp> {
-  using OpRewritePattern<CallIndirectOp>::OpRewritePattern;
 
-  LogicalResult matchAndRewrite(CallIndirectOp indirectCall,
-                                PatternRewriter &rewriter) const override {
-    // Check that the callee is a constant callee.
-    SymbolRefAttr calledFn;
-    if (!matchPattern(indirectCall.getCallee(), m_Constant(&calledFn)))
-      return failure();
+/// Fold indirect calls that have a constant function as the callee operand.
+LogicalResult CallIndirectOp::canonicalize(CallIndirectOp indirectCall,
+                                           PatternRewriter &rewriter) {
 
-    // Replace with a direct call.
-    rewriter.replaceOpWithNewOp<CallOp>(indirectCall, calledFn,
-                                        indirectCall.getResultTypes(),
-                                        indirectCall.getArgOperands());
-    return success();
-  }
-};
-} // end anonymous namespace.
-
-void CallIndirectOp::getCanonicalizationPatterns(RewritePatternSet &results,
-                                                 MLIRContext *context) {
-  results.add<SimplifyIndirectCallWithKnownCallee>(context);
-}
 
 //===----------------------------------------------------------------------===//
 // General helpers for comparison ops
 //===----------------------------------------------------------------------===//
 
 // Return the type of the same shape (scalar, vector or tensor) containing i1.
 static Type getI1SameShape(Type type) {
   auto i1Type = IntegerType::get(type.getContext(), 1);

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

void OpEmitter::genCodeForAddingArgAndRegionForBuilder(OpMethodBody &body,
   // Push all successors to the result.
   for (const NamedSuccessor &namedSuccessor : op.getSuccessors()) {
     body << formatv("  {0}.addSuccessors({1});\n", builderOpState,
                     namedSuccessor.name);
   }
 }
 
 void OpEmitter::genCanonicalizerDecls() {
-  if (!def.getValueAsBit("hasCanonicalizer"))
+  bool hasCanonicalizeMethod = false;
+  if (def.getValueAsBit("hasCanonicalizeMethod")) {
+    // static LogicResult FooOp::
+    // canonicalize(FooOp op, PatternRewriter &rewriter);
+    hasCanonicalizeMethod = true;
+    SmallVector<OpMethodParameter, 2> paramList;
+    paramList.emplace_back(op.getCppClassName(), "op");
+    paramList.emplace_back("::mlir::PatternRewriter &", "rewriter");
+    opClass.addMethodAndPrune("LogicalResult", "canonicalize",
+                              OpMethod::MP_StaticDeclaration,
+                              std::move(paramList));
+  }
+
+  if (!hasCanonicalizeMethod && !def.getValueAsBit("hasCanonicalizer"))
     return;
 
+  // Add a signature for getCanonicalizationPatterns if implemented by the
+  // dialect or if synthesized to call 'canonicalize'.
   SmallVector<OpMethodParameter, 2> paramList;
   paramList.emplace_back("::mlir::RewritePatternSet &", "results");
   paramList.emplace_back("::mlir::MLIRContext *", "context");
-  opClass.addMethodAndPrune("void", "getCanonicalizationPatterns",
-                            OpMethod::MP_StaticDeclaration,
-                            std::move(paramList));
+  auto kind = hasCanonicalizeMethod ? OpMethod::MP_Static
+                                    : OpMethod::MP_StaticDeclaration;
+  auto *method = opClass.addMethodAndPrune(
+      "void", "getCanonicalizationPatterns", kind, std::move(paramList));
+
+  // If synthesizing the method, fill it it.
+  if (hasCanonicalizeMethod)
+    method->body() << "  results.add(canonicalize);\n";
 }
 
 void OpEmitter::genFolderDecls() {
   bool hasSingleResult =
       op.getNumResults() == 1 && op.getNumVariableLengthResults() == 0;
 
   if (def.getValueAsBit("hasFolder")) {
     if (hasSingleResult) {
       opClass.addMethodAndPrune(