This is an archive of the discontinued LLVM Phabricator instance.

[mlir][docs] Update/Add documentation for MLIRs Pattern Rewrite infrastructure
ClosedPublic

Authored by rriddle on Aug 4 2020, 4:50 PM.

Details

Summary

This infrastructure has evolved a lot over the course of MLIRs lifetime, and has never truly been documented outside of rationale or proposals. This revision aims to document the infrastructure and user facing API, with the rationale specific portions moved to the Rationale folder and updated.

Depends On D85167

Diff Detail

Event Timeline

rriddle created this revision.Aug 4 2020, 4:50 PM
rriddle requested review of this revision.Aug 4 2020, 4:50 PM

I tried to touch on most of the important aspects, but let me know if I missed something. Thanks.

Thanks very much for this finally - this was long overdue. I skimmed through it - I think it's documenting most of the things. But I think it's also important to have a few lines where appropriate on what one can't do inside a pattern rewrite: in fact, this is typically what new users get wrong and source of many questions.

  1. IR shouldn't be mutated outside of the rewriter methods described here. An op for example can't be erased directly with its erase method.
  1. A replacement of uses or any other update to operands outside of the rewriter methods if performed will take effect but such a replacement will not be visible to the pattern rewriter and the op may not be reprocessed by the driver.

It would be good to factor these in somewhere if not already there.

flaub added a subscriber: flaub.Aug 5 2020, 11:16 AM
rriddle updated this revision to Diff 283758.Aug 6 2020, 3:57 PM

Address comments

ftynse added a comment.Aug 7 2020, 7:10 AM

I don't know how much of the rationale is reused from the previous version, so feel free to ignore comments about the parts you carried over.

mlir/docs/PatternRewriter.md
31

Nit: unmatched rparen

55

Maybe add something like: "the combined matchAndRewrite is useful to keep information that is not trivially recomputable between the matching and the rewriting phase"

63

[Not for the documentation]: can't we rather make the constructor RewritePattern a template that takes the op class and calls TemplateTy::getOperationName() itself?

106

Add that in-place updates are only allowed on the root operation? (Or has that been relaxed already?)

220

Nit: warning: unused variable `result'

235

Consider mentioning that dialect conversion supports type conversion

mlir/docs/Rationale/RationaleGenericDAGRewriter.md
18–19

This sounds a bit strange to me, especially the "replacing with another set", even though I understand what you want to say.

20–21

I'd consider using putting examples that are friendlier to "old-school" compiler people first, like peephole optimizations, canonicalization and inst-combine. I'm not even sure TF-related things should be mentioned in the generic document.

23

"MLIR graph" isn't a well-defined term AFAIK. I'd put just "optimization algorithms for IR at multiple levels".

29

Add "an affine loop"? Just to avoid the jump from TFLite to LLVM IR....

30

s/CUDA/PTX

33–34

The last clause does not seem to relate to the rest of the sentence.

39

"tile patterns" sounds wrong here. We don't necessarily want to tile the input graph with patterns, and we don't have a driver that does that.

42

performs

48

s/computation/replacement? or application?

54

I'm a bit confused by the "here are a few" passage, followed by "the most similar design...". It feels like there isn't going to be a list of related systems after all.

57

Nit: the comma looks unnecessary here

60

At the end of what?

65

drop "given"

68

Is this section intended to review related work, or to demonstrate how existing pattern-matching constructs can be expressed in MLIR?

84

Nit: can we reference a more recent clang version? :)

88

The text hasn't yet mentioned that source-to-source optimizers are tree-based.

94

This subsection does not explain how AST rewriting can be implemented in MLIR, unlike the previous section for constant folding.

113

I'd drop C++, it's a general OOP thing.

143

Nit: expand OTOH

149

Nit: consider pinning a specific revision rather than master, otherwise the line number will get quickly out-of-sync

171

Consider using the last name, or both first and last :)

187

Nit: let's use github links consistently

210

It would be great to specify why

212

We don't have runtime patterns in MLIR either, do we?

220

It would be great to have a section that explains how MLIR's approach addresses all of these shortcomings, point by point.

230

Which "these algorithms"?

231–233

Examples here sound too specific for folks who don't know how TF works inside (I would assume most compiler folks don't).

236

s/lowest cost/highest benefit. Although the optimization problems are isomorphic, let's not introduce confusing terminology.

239

Is it always locally optimal?

242

"DAG tile" again, it hasn't been defined. Googling for "dag tile" finds the current version of MLIR doc as one of the first results...

243

Nit: I think whoever wants to touch the internals of a compiler knows what NP-complete stands for. I'm also not 100% sure our variant of graph coverage problem is NP-complete (i.e., can be reduced to 3-SAT) or NP-hard. I'd use the latter to be safe unless there's a citation.

251

"very very"

261

This went from TF operations to "not apply for all hardware" really fast... I'd just stick with pattern being usable at all abstraction levels of MLIR, including target-independent and target-specific

263

Arguably, it is solvable, but there are multiple possible solutions with different trade-offs.

266

What "this project" ?

280

Let's not mention PHI nodes, MLIR doesn't have them and it may be confusing

285

Let's stick with "benefits".

bondhugula added inline comments.Aug 8 2020, 3:26 PM
mlir/docs/PatternRewriter.md
240

Somehow, this doesn't bring out the scheme that benefit comes into play while choosing among multiple available patterns for the *same* op.

mlir/docs/Rationale/RationaleGenericDAGRewriter.md
225

levels that it -> levels is that it

Nit: infra -> infrastructure

bondhugula added inline comments.Aug 8 2020, 3:31 PM
mlir/docs/PatternRewriter.md
31

Missing opening parenthesis.

I don't know how much of the rationale is reused from the previous version, so feel free to ignore comments about the parts you carried over.

Ah no worries, thanks for the comments. I was a bit hesitant at first, but now I'd rather just clean the doc up than let it be a relic and not valid for the current state of the project.

rriddle updated this revision to Diff 284483.Aug 10 2020, 12:54 PM
rriddle marked 45 inline comments as done.

Resolve comments

mlir/docs/PatternRewriter.md
63

You can't explicitly specify a template for a constructor, so there would be no way to achieve that.

106

You can in-place modify any operation you can call replaceOp/eraseOp on.

mlir/docs/Rationale/RationaleGenericDAGRewriter.md
39

DAG tiling is common phrasing in instruction selection literature, added a definition here.

212

We will soon (via PDL).

220

Is it fine to do this is a followup?

230

Which "these algorithms"?

242

Added a definition on first use in the introduction

ftynse accepted this revision.Aug 11 2020, 4:39 AM

Thanks, this looks great!

mlir/docs/Rationale/RationaleGenericDAGRewriter.md
220

Sure!

This revision is now accepted and ready to land.Aug 11 2020, 4:39 AM
This revision was landed with ongoing or failed builds.Aug 13 2020, 12:06 PM
This revision was automatically updated to reflect the committed changes.
mlir/docs/DialectConversion.md