This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add extensible dialects
ClosedPublic

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 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
19–23

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
19–23

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
19–23

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
19–23

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 ↗(On Diff #358607)
12 ↗(On Diff #358607)
15 ↗(On Diff #358607)
17–18 ↗(On Diff #358607)
36–37 ↗(On Diff #358607)
49–52 ↗(On Diff #358607)
96–106 ↗(On Diff #358607)

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 ↗(On Diff #358607)

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

134–135 ↗(On Diff #358607)

Why?

168–170 ↗(On Diff #358607)

Same comment here as operations.

224 ↗(On Diff #358607)

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 ↗(On Diff #358607)

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 ↗(On Diff #358607)

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?

math-fehr updated this revision to Diff 378961.Oct 12 2021, 4:00 AM
math-fehr marked 7 inline comments as done.
  • Add attributes support
  • Fix grammar and spelling in the documentation
  • Change the type and attribute interfaces to traits

All comments should be addressed now, besides two where I answered in the comments.

Hey @rriddle, a gentle ping on this.

Took another scan, thanks for the ping!

mlir/docs/ExtensibleDialects.md
96–106 ↗(On Diff #358607)

I see, so we would construct a bunch of dynamic operations up front and then register them later? Do we have any asserts that we actually add the dynamic operation to the dialect? (i.e. if we didn't call addDynamicOperation)

134–135 ↗(On Diff #358607)

I was considering if it would be better if we had an abstract interface for this data, that would not require them to be stored/represented in attributes. E.g. If I wanted to store some random swift/python/rust/etc. value, how would it be stored? is it required to be some form of pointer handle? Thinking about if the bindings for those languages don't force pointer handles, it might be more annoying to force going through an attribute.

mlir/include/mlir/IR/ExtensibleDialect.h
55–56

Can you document this method? And how it should be used.

56

nit: Drop all of the llvm:: from StringRef uses, StringRef is exported in the mlir namespace.

mlir/lib/IR/ExtensibleDialect.cpp
26–31

Please mark these as static and place at the top level, only classes should really go in anonymous namespaces.

30

nit: Drop the llvm:: here.

69–75

Can you just forward to the other ::get instead?

178–181

You should be able to call parser.getChecked<DynamicType>(typeDef, params) now.

This also seems to ignore the result of getChecked, what happens if it fails?

197–203

Same comment here as DynamicTypeDefinition.

306–311

Same comment here as DynamicTypeDefinition.

343–346
361–364

nit: Drop mlir:: and llvm:: here.

396–399

This should be in an anonymous namespace.

481–482

Can you spell out auto here? and also flip to use early exit?

if (!typeDef)

return llvm::None;
487

?

504–506

Same comment here as Type.

mlir/test/lib/Dialect/Test/TestAttributes.cpp
144–147 ↗(On Diff #378961)

nit: Use static methods at the top-level instead of anonymous namespaces.

258 ↗(On Diff #378961)

Prefer llvm_unreachable for cases like this.

rriddle added inline comments.Oct 28 2021, 10:20 AM
mlir/test/lib/Dialect/Test/TestTypes.cpp
222–226

Same comment about static vs anonymous namespace.

math-fehr added inline comments.Nov 9 2021, 6:26 PM
mlir/docs/ExtensibleDialects.md
134–135 ↗(On Diff #358607)

I see!
Do you have an intuition on what this interface should look like?
I feel like this interface would be exactly like an attribute, am I missing something?

rriddle added inline comments.Nov 15 2021, 12:20 AM
mlir/docs/ExtensibleDialects.md
134–135 ↗(On Diff #358607)

Do you have an intuition on what this interface should look like?

Not sure given that I have little experience with language binding things, but a more structured version of llvm::Any could work? Type-erased struct that has virtual? methods for accessing information about the value as necessary?

I feel like this interface would be exactly like an attribute, am I missing something?

The differences I see from Attribute are that:

  • We wouldn't need to unique each instance (performance win?)
  • We wouldn't need to define an Attribute compatible construct for each parameter, which might not be possible

Do we have attributes that cover all of the different things you might put in an attribute? For a python frontend, what values are being placed in attributes? What about for rust? etc.? For very primitive values I can see how we could reuse other existing attribute types, for more complex structs/etc. I'm not sure how that would work.

That being said, maybe using attributes are fine as an initial step.

math-fehr updated this revision to Diff 390180.Nov 27 2021, 6:21 PM
math-fehr marked 25 inline comments as done.

Address comments, and update to HEAD

math-fehr added a comment.EditedNov 27 2021, 6:23 PM

Add some inline responses.
I think that for now it would be better to stick with attributes as parameters to move forward, if that makes sense to you!

mlir/docs/ExtensibleDialects.md
96–106 ↗(On Diff #358607)

We don't assert that, but I can add a boolean in the definition that asserts that the operation is registered or not, and then assert in its destructor.
Does that makes sense?

134–135 ↗(On Diff #358607)

If we define an attribute for each language, we should be able to represent everything.
However yes, it is probably not optimal in terms of uniquing.
I'll keep it like this for a first step, since I'm not really sure about what would be the exact interface yet.

Sticking with attributes for now is fine. Will take another pass over in the next day or two, but should be good.

mlir/docs/ExtensibleDialects.md
14 ↗(On Diff #390180)

Might want to an an etc. in here given that attributes can also be defined at runtime.

120 ↗(On Diff #390180)

created by creating

nit: Can you rephrase?

152–153 ↗(On Diff #390180)

Can you align the params here and below?

189–202 ↗(On Diff #390180)

The similar boilerplate for parsing types defined in ODS can now be autogenerated, can we work this into that support?

Thanks! Will likely just need one more round.

mlir/include/mlir/IR/AttributeSupport.h
51 ↗(On Diff #390180)
mlir/include/mlir/IR/ExtensibleDialect.h
10–11

Can you beef this up? Or at least reference the more extensive website documentation?

41–42

Why here instead of at the top of the namespace?

43–45

Can you clean this up a bit? It doesn't really describe what a "dynamic type" is.

56

The op?

70
73

nit: Consider switching all of these Get the to Return the to be more consistent with the rest of the codebase.

103

Can you clean this up? It reads a bit weird. The same for the verifier and printer docs.

116–119
124–125

Can you beef this up a bit?

135

This should still call the verifier, but assert if invalid (otherwise it doesn't match what normal types do).

139–140

Please comment on the return if the type is invalid.

171–175

Same comments as the type section.

I would also move the attribute section before the type one, given A<T.

309–311

Can you beef this up?

334–348

Can you document these?

403

Drop the mlir::

467–477

Are all of these llvm:: necessary?

490–499

Why is this necessary? I would expect this to "just work".

mlir/lib/IR/ExtensibleDialect.cpp
229–230
386

Please update all usages of Identifier to StringAttr (they are the same now).

392–395

nit: Can you spell out all of these auto?

math-fehr updated this revision to Diff 408455.Feb 14 2022, 9:26 AM
math-fehr marked 24 inline comments as done.

Address comments

Sorry for not being active enough on this, I addressed all comments here.
I tried to add support for TableGen parser/printer generation, and added it in the generated parseType.
Though, I'm not really sure how to integrate it (or if I should integrate it) in generatedTypeParser, since the TableGen generation do not have access to the dialect if there is no TableGen type/attribute definition.

Otherwise, every comment should have been addressed!

mlir/include/mlir/IR/ExtensibleDialect.h
467–477

I removed the ones for DenseMap, though the ones for StringMap are necessary.
I'll make a PR for adding StringMap in the list of LLVM.h aliases, so I don't pollute this PR with this change (and the removing of llvm:: everywhere).

490–499

This is required here because Dialect.h is already overloading it for Dialect with this function:

template <typename T> struct isa_impl<T, ::mlir::Dialect> {
  static inline bool doit(const ::mlir::Dialect &dialect) {
    return mlir::TypeID::get<T>() == dialect.getTypeID();
  }
};

So the cast would call this instead of the usual ExtensibleDialect::classof.

rriddle accepted this revision.Feb 26 2022, 10:57 AM

Sorry for not being active enough on this, I addressed all comments here.
I tried to add support for TableGen parser/printer generation, and added it in the generated parseType.
Though, I'm not really sure how to integrate it (or if I should integrate it) in generatedTypeParser, since the TableGen generation do not have access to the dialect if there is no TableGen type/attribute definition.

I think it would be nice to work in if we can. Is the problem that we need to pass in the dialect to the generatedTypeParser method? We could always extend that method to pass in the dialect or something. Either way though, given that we support it in the default generation (which is something we are working towards having on by default), it isn't a huge deal.

I think we've hit a good enough starting point that we can land and start iterating in-tree. Thanks for all of the work on this!

This revision is now accepted and ready to land.Feb 26 2022, 10:57 AM

Thanks a lot for your reviews!
I'll try to see if I manage to add printing/parsing support in the generatedTypeParser, and I'll try to make a patch for that soon!

Would it be possible to land it for me? I do not have commit access.

Can you rebase and re-upload?

math-fehr updated this revision to Diff 411903.Feb 28 2022, 2:40 PM

Rebase to main

math-fehr updated this revision to Diff 411944.Feb 28 2022, 7:01 PM

Update to latest commit

math-fehr updated this revision to Diff 411946.Feb 28 2022, 7:12 PM

Apply clang-format

I rebased it to the latest commit, though I'm still investigating why the windows build is failing.

math-fehr updated this revision to Diff 412360.Mar 2 2022, 2:20 AM

Fix bug in inheritance, and remove unnecessary code

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 2:20 AM
This revision was landed with ongoing or failed builds.Mar 2 2022, 12:43 PM
Closed by commit rGdbe9f0914fcf: [mlir] Add extensible dialects (authored by math-fehr, committed by rriddle). · Explain Why
This revision was automatically updated to reflect the committed changes.

Hi, I've just reverted this as it's been causing build failures in Flang's Windows buildbot: flang-x86_64-windows. I don't have a Windows machine to reproduce myself, but similar build failure was reported by the pre-commit CI:

FAILED: tools/mlir/lib/IR/CMakeFiles/obj.MLIRIR.dir/ExtensibleDialect.cpp.obj
sccache C:\BuildTools\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=1 -DMLIR_ROCM_CONVERSIONS_ENABLED=1 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\mlir\lib\IR -IC:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR -Iinclude -IC:\ws\w4\llvm-project\premerge-checks\llvm\include -IC:\ws\w4\llvm-project\premerge-checks\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Fotools\mlir\lib\IR\CMakeFiles\obj.MLIRIR.dir\ExtensibleDialect.cpp.obj /Fdtools\mlir\lib\IR\CMakeFiles\obj.MLIRIR.dir\ /FS -c C:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR\ExtensibleDialect.cpp
C:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR\ExtensibleDialect.cpp(159): error C2672: 'mlir::Type::hasTrait': no matching overloaded function found
C:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR\ExtensibleDialect.cpp(159): error C3207: 'mlir::Type::hasTrait': invalid template argument for 'Trait', class template expected
C:\ws\w4\llvm-project\premerge-checks\mlir\include\mlir/IR/Types.h(174): note: see declaration of 'mlir::Type::hasTrait'
C:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR\ExtensibleDialect.cpp(278): error C2672: 'mlir::Attribute::hasTrait': no matching overloaded function found
C:\ws\w4\llvm-project\premerge-checks\mlir\lib\IR\ExtensibleDialect.cpp(278): error C3207: 'mlir::Attribute::hasTrait': invalid template argument for 'Trait', class template expected
C:\ws\w4\llvm-project\premerge-checks\mlir\include\mlir/IR/Attributes.h(91): note: see declaration of 'mlir::Attribute::hasTrait'

Thanks for working on this and please let me know if I can help in any way!

Thanks for taking care of this!
I'll try to figure out why windows is failing, and fix this in the next couple of days!

math-fehr reopened this revision.Apr 21 2022, 1:28 PM
This revision is now accepted and ready to land.Apr 21 2022, 1:28 PM
math-fehr updated this revision to Diff 424279.Apr 21 2022, 1:29 PM

Update to latest version

The two suggested changes (and the accompanying knock-on changes of those) fixed the build on windows. Don't ask me why, it just does. It also better aligns with how we layer Traits anyways, so seems like a better state overall.

mlir/include/mlir/IR/ExtensibleDialect.h
132–134
276–278
math-fehr updated this revision to Diff 424341.Apr 21 2022, 5:19 PM

Fix compilation on windows machines

Windows build looks good now, can your reformat/rebase/upload? I can land for you afterwards.

rriddle added a comment.EditedApr 22 2022, 11:02 AM

Also I've recently added real docs for defining dialects (https://mlir.llvm.org/docs/DefiningDialects/), in a follow up patch can we merge the ExtensibleDialects.md doc into that one?

math-fehr updated this revision to Diff 424802.Apr 24 2022, 3:57 PM

Update to latest MLIR, and format files

Everything should be ready now! Could you land this for me?
I'll merge the dialect documentation in a following patch!

math-fehr updated this revision to Diff 424931.Apr 25 2022, 9:14 AM

Rename dynamic attributes/types/ops in test dialect

I just changed the names of the types, attributes, and ops in the test dialect, to address the comment of D124353.
The patch is otherwise ready if the CI passes.

This revision was automatically updated to reflect the committed changes.