Hi all,
In another random flight of fancy, I’ve been itching to make it easier and prettier to implement canonicalization hooks for the “simple case”. Right now, you have to do something like in TableGen:
def AttachOp : FIRRTLOp<"attach"> {
...
let hasCanonicalizer = 1;
}
and then implement it like this:
void AttachOp::getCanonicalizationPatterns(OwningRewritePatternList &results,
MLIRContext *context) {
struct Folder final : public OpRewritePattern<AttachOp> {
using OpRewritePattern::OpRewritePattern;
LogicalResult matchAndRewrite(AttachOp op,
PatternRewriter &rewriter) const override {
// Single operand attaches are a noop.
if (op.getNumOperands() <= 1) {
rewriter.eraseOp(op);
return success();
}
// ...other stuff...
return failure();
}
};
results.insert<Folder>(context);
}
The problem is that this requires a ton of boilerplate - a nested class, a super indented set of logic, etc. There are other variants of these (declaring the struct outside of the method; then optionally declaring matchAndRewrite out of line) but they are all a ton of boilerplate.
I think we can get to something much better (for the simple matchAndRewrite case). I think we can get to:
def AttachOp : FIRRTLOp<"attach"> {
...
let hasSimpleCanonicalizer = 1;
}
and then implement it like this, which eliminates all the boilerplate [1]:
LogicalResult AttachOp::canonicalize(AttachOp op, PatternRewriter &rewriter) {
// Single operand attaches are a noop.
if (op.getNumOperands() <= 1) {
rewriter.eraseOp(op);
return success();
}
// Other stuff.
return failure();
}
I prototyped this (without tablegen support) in this patch, which has both a closure approach (not as nice) and the static method approach.
Does this seem like a promising thing to work on, or does anyone have any specific concerns? This wouldn’t remove any of the existing support for the general case.
-Chris
[1] in case you’re curious, this is implemented as a static method taking an ‘op’ instead of an instance method because you have to delete this, which would be UB for an instance method.
