Page MenuHomePhabricator

[mlir][Pattern] Create a new IRRewriter class to enable sharing code with pattern rewrites
ClosedPublic

Authored by rriddle on Jan 13 2021, 2:52 PM.

Details

Summary

This revision adds two new classes, RewriterBase and IRRewriter. RewriterBase is a new shared base class between IRRewriter and PatternRewriter. PatternRewriter will continue to be the base class used to perform rewrites within a rewrite pattern. IRRewriter on the other hand, is a new class that allows for tracking IR rewrites from outside of a rewrite pattern. In this revision all of the old API from PatternRewriter is moved to RewriterBase, but the distinction between IRRewriter and PatternRewriter is kept on the chance that a necessary API divergence happens in the future.

Currently if you want to have some utility that transforms a piece of IR and share it between pattern and non-pattern code, you have to duplicate it. This revision enables the creation of utilities that can be invoked from rewrite patterns and normal transformation code:

c++
void someSharedUtility(RewriterBase &rewriter, ...) {
  // Some interesting IR mutation here.
}

// Some RewritePattern
LogicalResult MyPattern::matchAndRewrite(Operation *op, PatternRewriter &rewriter) {
  ...
  someSharedUtility(rewriter, ...);
  ...
}

// Some Pass
void MyPass::runOnOperation() {
  ...
  IRRewriter rewriter(...);
  someSharedUtility(rewriter, ...);
}

Depends On D94632

Diff Detail

Event Timeline

rriddle created this revision.Jan 13 2021, 2:52 PM
rriddle requested review of this revision.Jan 13 2021, 2:52 PM

I went with IRRewriterImpl as this seems close to a SmallVectorImpl situation, but happy to rename to something else if there are suggestions.

bollu added a subscriber: bollu.Jan 15 2021, 7:38 PM

Out of curiosity: may I please have some context for what this base class is being created for? Ie, what consumers other than the existing PatternRewriter will soon exist? :)

mlir/lib/IR/PatternMatch.cpp
315

Nit: spuriously deleted line?

rriddle added a comment.EditedJan 15 2021, 8:25 PM

Out of curiosity: may I please have some context for what this base class is being created for? Ie, what consumers other than the existing PatternRewriter will soon exist? :)

There are existing use cases, i.e. if you want to have some utility that transforms a piece of IR and share it between pattern and non-pattern code you currently have to duplicate it. What you would be able to do after this revision is share it by using an IRRewriterImpl as the driver for the mutation:

void someSharedUtility(IRRewriterImpl &rewriter, ...) {
  // Some interesting IR mutation here.
}

// Some RewritePattern
LogicalResult MyPattern::matchAndRewrite(Operation *op, PatternRewriter &rewriter) {
  ...
  someSharedUtility(rewriter, ...);
  ...
}

// Some Pass
void MyPass::runOnOperation() {
  ...
  IRRewriter rewriter(...);
  someSharedUtility(rewriter, ...);
}
mehdi_amini accepted this revision.Jan 23 2021, 12:06 PM
This revision is now accepted and ready to land.Jan 23 2021, 12:06 PM
jpienaar accepted this revision.Jan 25 2021, 10:59 AM

Out of curiosity: may I please have some context for what this base class is being created for? Ie, what consumers other than the existing PatternRewriter will soon exist? :)

There are existing use cases, i.e. if you want to have some utility that transforms a piece of IR and share it between pattern and non-pattern code you currently have to duplicate it. What you would be able to do after this revision is share it by using an IRRewriterImpl as the driver for the mutation:

I think this would be good to capture in change description more explicitly (I liked the small example and what you wrote here for an extensive commit message :))

mlir/include/mlir/IR/PatternMatch.h
425

Does the naming convention here mirror SmallVector / SmallVectorImpl? "impl" makes me think of an implementation detail vs a common base class. This is a "listening builder" or "observed builder" base class (well technically an observable, it is only observed with listeners registered ...)

597

Context is a bit overloaded. Is this a hard requirement or a nice-to-have? E.g., can I mix and match but a bad idea to do so, or just preferred?

rriddle updated this revision to Diff 320654.Mon, Feb 1, 6:06 PM
rriddle marked 2 inline comments as done.

Address feedback

mlir/include/mlir/IR/PatternMatch.h
425

Yeah, the Impl here mirror'd SmallVectorImpl. I couldn't think of a better name at the time, but switched to using RewriterBase instead.

597

It's a hard requirement, given that the rewriter you aren't using wouldn't get the notifications for rewrites you are doing.

rriddle edited the summary of this revision. (Show Details)Mon, Feb 1, 6:06 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.