Page MenuHomePhabricator

[mlir] Add extensible dialects
Needs ReviewPublic

Authored by math-fehr on Jun 18 2021, 10:59 AM.

Details

Summary

Depends on D104534
Add support for extensible dialects, which are dialects that can be
extended at runtime with new operations and types.

These operations and types cannot at the moment implement traits
or interfaces.

Diff Detail

Event Timeline

math-fehr created this revision.Jun 18 2021, 10:59 AM
math-fehr requested review of this revision.Jun 18 2021, 10:59 AM
math-fehr edited the summary of this revision. (Show Details)Jun 18 2021, 11:00 AM
math-fehr added a reviewer: mehdi_amini.
math-fehr updated this revision to Diff 353668.Jun 22 2021, 9:10 AM

Remove use of std::vector in favor of SmallVector

math-fehr updated this revision to Diff 354268.Jun 24 2021, 8:52 AM

Add new useful methods on ExtensibleDialect.
Also, fix cast on ExtensibleDialect.

Fix include guard name and use const_cast instead of an API change

math-fehr edited the summary of this revision. (Show Details)Jun 24 2021, 11:22 AM
math-fehr updated this revision to Diff 354688.Jun 26 2021, 8:27 AM

Return nullptr instead of a FailureOr in lookupTypeDefinition

Sorry for the delay!

Can you write up a doc (in the docs/ folder) detailing everything here? It would make it easier to understand the design, how it is intended to be used, etc.

mlir/include/mlir/IR/IsDynamicInterfaces.td
20–24

This could just be a trait? Is there a specific reason you need an interface?

Sorry for the delay!

Can you write up a doc (in the docs/ folder) detailing everything here? It would make it easier to understand the design, how it is intended to be used, etc.

No problem!
I'll write something this week.

mlir/include/mlir/IR/IsDynamicInterfaces.td
20–24

Yes, a trait would work.
However, it seems that MLIR does not really support type traits. I found a TypeTrait class, but no hasTrait function on the Type class, nor ways to define type traits in ODS.
Is there anything I missed, or maybe a patch is needed to implement them?

rriddle added inline comments.Jun 28 2021, 1:06 PM
mlir/include/mlir/IR/IsDynamicInterfaces.td
20–24

We just need a patch to implement the remaining accessors (interfaces use traits internally). Do you want to prepare a patch for that? It would be very welcome.

math-fehr marked an inline comment as done.Jun 28 2021, 1:10 PM
math-fehr added inline comments.
mlir/include/mlir/IR/IsDynamicInterfaces.td
20–24

Sure! I'll write a patch to add that.

math-fehr updated this revision to Diff 355021.Jun 28 2021, 1:37 PM
math-fehr marked an inline comment as done.

Fix memory error by allocating the dialect name in the MLIRContext.

math-fehr updated this revision to Diff 358607.Jul 14 2021, 7:52 AM

Add a file in docs/ documenting the patch

Sorry for the delay!

rriddle added inline comments.Aug 10 2021, 6:26 PM
mlir/docs/ExtensibleDialects.md
5–6
12
15
17–18
36–37
49–52
96–106

It feels like DynamicOpDefinition::get should not be accessible, and instead just an implementation detail of an overload to addDynamicOperation. Is there a benefit to not having this API directly on ExtensibleDialect?

108–109

If you did the above, you wouldn't need this at all. It would be guaranteed on construction.

134–135

Why?

168–170

Same comment here as operations.

224

Can you add a section/support for attributes?

Sorry for the delay!

I responded to two inline comments about the design choices.
I'll fix the other comments once I have some time!

mlir/docs/ExtensibleDialects.md
96–106

So there are two problems here:

  • One, we have many optional fields (such as the printer, parser, fold hook function, and the canonicalization patterns). I thought that having a builder pattern would be better, but it seems indeed that having multiple overloaded functions that would solve most cases would be best if this was only the problem.
  • The main problem is that I think we want to allow cyclic dependencies between the operation definitions. For instance, an op A and B could both refer to the other op in their verifiers. In order to do that efficiently, we need that A knows the TypeID of B, and vice-versa. This can be done with DynamicOpDefinition, since it already allocated the TypeID.

Do you think that this makes sense?

134–135

Attributes and Types in MLIR are defined with C++ classes to hold arbitrary data.
However, to represent types (and attributes) defined at runtime, we need a unified way that would allow to hold arbitrary data.
One solution is to represent parameters of types defined at runtime by an array of Attribute. This let us hold arbitrary data (given that the corresponding Attribute is already defined).
I don't have any other solution for this problem, what are your thoughts?