Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
rriddle added inline comments.Oct 28 2021, 10:20 AM
mlir/test/lib/Dialect/Test/TestTypes.cpp
390–394

Same comment about static vs anonymous namespace.

math-fehr added inline comments.Nov 9 2021, 6:26 PM
mlir/docs/ExtensibleDialects.md
135–136

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
135–136

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
97–107

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?

135–136

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
15

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

121

created by creating

nit: Can you rephrase?

153–154

Can you align the params here and below?

190–203

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
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.