This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Introduce Transform dialect
ClosedPublic

Authored by ftynse on Apr 5 2022, 8:35 AM.

Details

Summary

This dialect provides operations that can be used to control transformation of
the IR using a different portion of the IR. It refers to the IR being
transformed as payload IR, and to the IR guiding the transformation as
transform IR.

The main use case for this dialect is orchestrating fine-grain transformations
on individual operations or sets thereof. For example, it may involve finding
loop-like operations with specific properties (e.g., large size) in the payload
IR, applying loop tiling to those and only those operations, and then applying
loop unrolling to the inner loops produced by the previous transformations. As
such, it is not intended as a replacement for the pass infrastructure, nor for
the pattern rewriting infrastructure. In the most common case, the transform IR
will be processed and applied to payload IR by a pass. Transformations
expressed by the transform dialect may be implemented using the pattern
infrastructure or any other relevant MLIR component.

This dialect is designed to be extensible, that is, clients of this dialect are
allowed to inject additional operations into this dialect using the newly
introduced in this patch TransformDialectExtension mechanism. This allows the
dialect to avoid a dependency on the implementation of the transformation as
well as to avoid introducing dialect-specific transform dialects.

See https://discourse.llvm.org/t/rfc-interfaces-and-dialects-for-precise-ir-transformation-control/60927.

Diff Detail

Event Timeline

ftynse created this revision.Apr 5 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald Transcript
ftynse requested review of this revision.Apr 5 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 8:35 AM

Thanks for pushing on this @ftynse , this looks great!

First round of comments.

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
48

This is the mechanism also used by TF right?
Seems quite mundane in the existing core codebase, I am surprised no one in core uses this already besides Dialect::getOperations.
I guess no one needed dynamic registration of ops in core yet.

53

nit: replicate the traditional C++14 -> C++17 comment ?

58

nit: Declare that *this* Transform dialect *extension* ?

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
31

nit: to the payload IR

35

nit grammo: how -> what ... may look like

53

Nice!

176

nit

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
24

Consider reminding readers that the signature for TransformOp::apply is

apply(transform::TransformResults &results, transform::TransformState &state)

and maybe describe a little more the life of each parameter in a notional C++ impl of chained applies?

Having a small example of 2-3 calls showing how things would look like in C++ would help reading comprehension IMO, atm the transform ops hide this behind a level of indirection that is not trivial.

28

grammo

75

nit: operationsMapping ?

mlir/lib/Dialect/Transform/IR/TransformDialect.cpp
16

nit

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
56

Wouldn't you be better off with a reverse map?
The expected uniqueness at runtime would also be clear directly in the class doc.

mlir/test/Dialect/Transform/test-dialect-injection.mlir
7

nit: newline before this // CHECK and the next for improved readability

mlir/test/Dialect/Transform/test-interpreter.mlir
16

typo: associated here and at production site.

mlir/test/lib/Dialect/Test/TestTransformDialectExtension.cpp
54 ↗(On Diff #420525)

could you please add a short doc of the 2 behaviors here?
or better rename in a very pedantic fashion, e.g. transform.test_produce_42_or_forward_operand ?

Wasn't obvious to me what to look for in the test.

64 ↗(On Diff #420525)

same here: transform.test_consume_operand_if_associated_with_parameter_or_fail

70 ↗(On Diff #420525)

nit: typo

mlir/test/lib/Dialect/Test/TestTransformDialectExtension.td
40 ↗(On Diff #420525)

nit

mlir/test/lib/Dialect/Test/TestTransformDialectInterpreter.cpp
40 ↗(On Diff #420525)

nit: I think you can return signalPassFailure(); and gain a few lines

56 ↗(On Diff #420525)

nit

Nice!

Do we not want to introduce at least one op to the Transform dialect, transform.sequence? I might anticipate in practice using func

func.func @my_sequence(%arg: !pdl.operation) -> (!pdl.operation) {
  // do some transformations
}

But since we're moving away from func.func being general-purpose, I think at least one function-like op in transform is warranted.

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
47

Should we static_assert that the ops implement the transform interface?

48

TF uses a more roundabout mechanism because it was introduced before DialectExtension (but fundamentally, yes)

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
18

unneeded forward declaration (provided through OpDefinition.h)

56

static constexpr Value

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.td
52

Any documentation for this trait? Is it used anywhere?

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
74

operations.erase(value)?

91

Why can't the update be done in-place?

105
mlir/test/lib/Dialect/Test/TestTransformDialectExtension.cpp
68 ↗(On Diff #420525)
69–71 ↗(On Diff #420525)
rriddle requested changes to this revision.Apr 5 2022, 1:59 PM

A major thing that is still unclear for me is what the next steps of this are. It isn't clear yet how this is going to be used, what the constraints are, etc. This feels like one piece of the solution and it's hard to consider the impact of this without first discussing/seeing the rest of the story.

This revision now requires changes to proceed.Apr 5 2022, 1:59 PM
ftynse updated this revision to Diff 420892.Apr 6 2022, 8:38 AM
ftynse marked 24 inline comments as done.

Address review.

ftynse marked 2 inline comments as done.Apr 6 2022, 8:52 AM

Nice!

Do we not want to introduce at least one op to the Transform dialect, transform.sequence? I might anticipate in practice using func

func.func @my_sequence(%arg: !pdl.operation) -> (!pdl.operation) {
  // do some transformations
}

But since we're moving away from func.func being general-purpose, I think at least one function-like op in transform is warranted.

There is no plan to use func. Specific ops deserve their own discussion (there were two options for sequence on the forum) so I kept them out of the first patch that sets up the infra for the dialect.

A major thing that is still unclear for me is what the next steps of this are. It isn't clear yet how this is going to be used, what the constraints are, etc. This feels like one piece of the solution and it's hard to consider the impact of this without first discussing/seeing the rest of the story.

The next steps are adding the ops to drive the transformations we already have in a more prescriptive and fine-grained fashion than we currently can. For example, we have the conversion from parallel loops to GPU that indiscriminately applies to all parallel loops, we have op-level tiling that either indiscriminately applies to all ops of a specific kind or is configured by a C++ callback invisible at the IR level, we have (affine) loop transformations in a similar state, etc. Instead, we can, for example, match specific ops using PDL and call transformations (more complex than just one pattern rewrite) on them. I seem to remember discussing this at the ODM.

I would really like us to follow our own guidelines and build things progressively in tree, and it is hard to foresee how a piece of infrastructure will end up being used. Do you have specific concerns?

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
47

I don't think we can static_assert, interface implementations can be added separately from the op class. We can try having a regular assertion though.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
24

Added a longer description of the lifetime. I am reluctant to copy the signature over to the doc: the doc will likely bitrot when the signature is changed.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.td
52

Not yet, removed.

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
56

You mean storing the reverse map in the class? It would make getPayloadOps also scan the entire map. Or do you want to store both?

91

The callback may return nullptr, in which case the op should be just dropped from the list. We could update to null in-place and then repack the vector, but that would still require an allocation or multiple copies.

Nice!

Do we not want to introduce at least one op to the Transform dialect, transform.sequence? I might anticipate in practice using func

func.func @my_sequence(%arg: !pdl.operation) -> (!pdl.operation) {
  // do some transformations
}

But since we're moving away from func.func being general-purpose, I think at least one function-like op in transform is warranted.

There is no plan to use func. Specific ops deserve their own discussion (there were two options for sequence on the forum) so I kept them out of the first patch that sets up the infra for the dialect.

A major thing that is still unclear for me is what the next steps of this are. It isn't clear yet how this is going to be used, what the constraints are, etc. This feels like one piece of the solution and it's hard to consider the impact of this without first discussing/seeing the rest of the story.

The next steps are adding the ops to drive the transformations we already have in a more prescriptive and fine-grained fashion than we currently can. For example, we have the conversion from parallel loops to GPU that indiscriminately applies to all parallel loops, we have op-level tiling that either indiscriminately applies to all ops of a specific kind or is configured by a C++ callback invisible at the IR level, we have (affine) loop transformations in a similar state, etc. Instead, we can, for example, match specific ops using PDL and call transformations (more complex than just one pattern rewrite) on them. I seem to remember discussing this at the ODM.

I would really like us to follow our own guidelines and build things progressively in tree, and it is hard to foresee how a piece of infrastructure will end up being used. Do you have specific concerns?

Those guidelines are generally when there is an agreed upon direction, but I don't think that's the case here. I don't think much of how transformations would actually be controlled, the constraints on the interpreter driving them, or how pervasive we want this to be within upstream dialects has been discussed. The RFC is very light on these details and mostly only discusses the representational aspects.

Let's discuss this tomorrow in person?
It would be great if you come up with very specific questions River, I thought the last ODM provided already a good basis to start moving forward in-tree with this?

The way I'd look at it is "how intrusive" will this be for other in-tree things? Will it be coupled? Will this put restriction on the other dialect and things in terms of design, evolution, maintenance? Can we delete this entirely in 2 months easily?

mlir/lib/Dialect/Transform/IR/TransformInterfaces.cpp
56

Store both

ftynse added a comment.Apr 7 2022, 5:37 AM

I thought the last ODM provided already a good basis to start moving forward in-tree with this?

I had the same impression, too.

Some rough ideas about the specific issues below.

The way I'd look at it is "how intrusive" will this be for other in-tree things?

The goal is to make this minimally intrusive, hence making it a separate dialect, figuring out the extension mechanism (instead of the originally considered interface registration), etc. We may want to add some features to other parts of the infrastructure, specifically to simplify the connection to patterns/PDL, but it should not affect their main design ideas.

Will it be coupled? Will this put restriction on the other dialect and things in terms of design, evolution, maintenance?

No more than a dependency on any other dialect, likely even less thanks to the extension+interface mechanism. The main coupling risk I see is having new transformations being inadvertently implemented in a way that only works within the transform dialect framework. I don't consider this to be a significant risk: factoring out transformation implementations into independent functions, patterns or other structuring concepts is just decent software engineering, lack of it should be evident at code review.

This approach to transformations is opt-in for dialects to use. Behavioral or API changes will cause some churn in the dialects that opted in. Same is true for any other dialect.

Can we delete this entirely in 2 months easily?

This is the corollary of coupling. It should be relatively easy. Not rm -rf lib/Dialect/Transform because the extension mechanism will make it possible for transformation implementations to live in other dialects, but should be more or less moving over any functionality that is still necessary to other files and removing the remnants.

ftynse updated this revision to Diff 421179.Apr 7 2022, 6:08 AM
ftynse marked 2 inline comments as done.

Adapt to the new TypeID style.

rriddle added inline comments.Apr 7 2022, 10:35 AM
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
48

We could likely just selectively expose transformDialect->addOps and use it here. With that we could probably just drop this methods and anchor solely on registerTransformOps.

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
36

?

38

Missing mlir tag here.

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
14

These bottom two includes likely aren't necessary.

65–66

Can you provide an example of this?

100–102

Is the position here the result number?

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.td
21

Please indent this.

37–43
47–48
mlir/test/lib/Dialect/Test/TestTransformDialectExtension.td
1 ↗(On Diff #421179)

Do these files need to be in the Test/ directory? Could these be defined in a Transforms/ dialect directory?

ftynse updated this revision to Diff 421509.Apr 8 2022, 6:29 AM
ftynse marked 12 inline comments as done.

Address review and improve documentation.

nicolasvasilache accepted this revision.Apr 12 2022, 8:21 AM

I believe discussion occurred and concerns have been addressed.

Mogball accepted this revision.Apr 13 2022, 11:01 AM

LGTM.

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.h
47

Yeah that sounds good. It doesn't really make sense to allow any random op to be added to the dialect when the dialect expects its ops to have some semantics (according to the interface)

mlir/lib/Dialect/Transform/IR/TransformDialect.cpp
16

Still no newline :(

mlir/test/lib/Dialect/Transform/TestTransformDialectExtension.cpp
60

This seems a little hacky (even for a test). Why not create an op with an i32 attribute?

rriddle accepted this revision.Apr 13 2022, 12:53 PM

(sorry, traveling). This looks like a good starting point, thanks for all of the documentation.

mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
26–27

This kind of seems like a large initial starting area. Why not incrementally build/decide this list?

54

Is there a set of naming conventions on the operations within this dialect?

mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.td
44

This needs to be fully scoped.

mlir/test/lib/Dialect/Transform/TestTransformDialectExtension.cpp
60

+1 here.

This revision is now accepted and ready to land.Apr 13 2022, 12:53 PM
ftynse updated this revision to Diff 422808.Apr 14 2022, 4:17 AM
ftynse marked 8 inline comments as done.

Address review.

ftynse added inline comments.Apr 14 2022, 4:18 AM
mlir/include/mlir/Dialect/Transform/IR/TransformDialect.td
26–27

Reducing to the first case only (structured ops). One of the first things we will support is tiling, and it works through another interface that is implemented by ops in several dialects.

54

Now there is. :) Added at the end of the section, after the extensibility paragraph.

This revision was landed with ongoing or failed builds.Apr 14 2022, 4:49 AM
This revision was automatically updated to reflect the committed changes.