Page MenuHomePhabricator

[mlir] Add an out-of-tree dialect example
ClosedPublic

Authored by Kayjukh on Mar 31 2020, 4:01 AM.

Details

Summary

This adds a minimal out-of-tree dialect template which can be used to start work on a standalone dialect implementation without having to integrate it in the main LLVM tree.

It mostly sets up the directory structure and provides CMakeLists.txt files to build a dialect library, an opt-like tool to operate on that dialect as well as tests. It could be expanded in the future to add examples of more user-defined operations, types, attributes, generated enums, transforms, etc. and linked to a tutorial.

Diff Detail

Event Timeline

Kayjukh created this revision.Mar 31 2020, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 4:01 AM
Kayjukh edited the summary of this revision. (Show Details)Mar 31 2020, 4:01 AM
Kayjukh retitled this revision from Add an out-of-tree dialect example to [mlir] Add an out-of-tree dialect example.
stephenneuendorffer requested changes to this revision.Mar 31 2020, 12:08 PM

Thanks for taking the lead on this!

mlir/examples/standalone/README.md
6

This needs some documentation before it goes in.

mlir/examples/standalone/include/Standalone/StandaloneOps.td
14

I think this actually needs to define an OP and use it.

mlir/examples/standalone/standalone-opt/standalone-opt.cpp
58–97

I'm a little concerned about the amount of copy-pasta here: it would be nice if more of this could be pushed into MlirOptMain., but it's unclear what the most useful line is to draw around the 'shared' bits vs. the boilerplate that just has to get copied. I don't think this necessarily has to get changed before it goes in.

This revision now requires changes to proceed.Mar 31 2020, 12:09 PM

This is fantastic, thanks.

  1. Would it be possible to use libMLIR.so and a plugin library?
  2. LLVM opt has a -load option that allows one to load new passes. So I can do opt -load=libjulia.so and load all the Julia passes and then run them as part of opt.
Kayjukh updated this revision to Diff 254048.Mar 31 2020, 5:16 PM
Kayjukh marked an inline comment as done.
Kayjukh edited the summary of this revision. (Show Details)

This update adds build instructions to the README file as well as an example of a user-defined operation.

The custom operation does not use the declarative TableGen assembly form even though it could really benefit from it. I think it illustrates the more general case, but I am open to further discussing this point.

rriddle requested changes to this revision.Mar 31 2020, 5:23 PM

Looks great.

mlir/examples/standalone/include/Standalone/StandaloneDialect.h
18

Please use the dialect declaration generator instead. i.e. -gen-dialect-decls

mlir/examples/standalone/include/Standalone/StandaloneOps.td
19

This looks like it needs to be wrapped at 80 characters.

mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp
16

nit: Please use /// for top-level comments

23

Should you be using the declarative assembly format instead?

https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format

This revision now requires changes to proceed.Mar 31 2020, 5:23 PM
Kayjukh accepted this revision.Mar 31 2020, 5:28 PM
Kayjukh marked 3 inline comments as done.
Kayjukh added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
58–97

I think the current split makes most sense because it separates user input processing and tool-specific customization points (such as which dialects to enable in the final executable) from the core MlirOptMain.

There is a lot of copy-pasting in this case because I wanted to stick to the current mlir-opt behavior.

This is fantastic, thanks.

  1. Would it be possible to use libMLIR.so and a plugin library?
  2. LLVM opt has a -load option that allows one to load new passes. So I can do opt -load=libjulia.so and load all the Julia passes and then run them as part of opt.

I'm not sure I understand. Do you mean adding a dynamic plugin-loading infrastructure to the standalone-opt example tool?

Kayjukh updated this revision to Diff 254055.Mar 31 2020, 5:41 PM
Kayjukh marked 3 inline comments as done.

The main dialect include file now includes the tablegen'd declaration of the dialect instead of providing it manually.

Kayjukh accepted this revision.Mar 31 2020, 5:47 PM
Kayjukh marked an inline comment as done.
mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp
23

I considered using the declarative syntax at first, but I think using a custom parser and printer illustrates a more general (albeit rare) case where you might want to do more during parsing than what is done by the generated parser.

But as I said in a previous update comment, I am very much open to discussion on this point. I don't know if it is relevant to account for such corner cases in an example dialect.

rriddle added inline comments.Mar 31 2020, 6:19 PM
mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp
23

It all depends on what you are trying to convey with your example. Given that examples are generally used as a template, I would rather do the right thing instead. For example, you could have defined the operation classes manually in C++ but you didn't. Why shouldn't the same apply to the assembly format?

This LGTM other than inline comments, you likely should try to wait for River's approval as well.

mlir/examples/standalone/README.md
11

Why is -DCMAKE_LINKER= needed here?

18

This is beyond this revision, but I think adding an option for having an external FileCheck just like for -DLLVM_EXTERNAL_LIT would help here.

mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp
23

+1 with River here. If this is a template I would go with the most idiomatic way (otherwise you could just not use ODS at all).

mlir/examples/standalone/standalone-opt/CMakeLists.txt
33

The list seems much much too long here, why do you need LLVMAsmParser for instance?
I'm concerned about how this is unmaintainable.

Kayjukh updated this revision to Diff 254153.Apr 1 2020, 3:52 AM
Kayjukh marked 10 inline comments as done.

The custom standalone.foo operation now makes use of the declarative assembly syntax form instead of manually providing a parser and a printer.

I also removed an unneeded CMake definition in the build instructions and significantly reduced the list of libraries that get linked to standalone-opt. Since this list was a copy-pasted version of the CMakeLists.txt from the current mlir-opt tool, it might be a good idea to do a cleanup pass over the latter one.

Kayjukh accepted this revision.Apr 1 2020, 11:25 AM
Kayjukh added inline comments.
mlir/examples/standalone/README.md
11

It isn't. This was an artifact from one of my previous tests.

18

Yes, this would definitely help.

mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp
23

Ok, yes, makes sense.

mlir/examples/standalone/standalone-opt/CMakeLists.txt
33

This was originally a simple copy-and-paste from the mlir-opt CMakeLists.txt file, which for some reason links against all of these libraries.

I updated the list to include the minimum amount of libraries so as not to produce a linker error and still make the tests pass. The user of the template will then be free to link to whatever additional libraries may be needed.

Kayjukh updated this revision to Diff 254473.Apr 2 2020, 3:34 AM

Further simplify the standalone-opt CMakeLists.txt by reflecting the changes on master for mlir-opt (commit b8c260c38d0ae93f8ae037fc1e9b94695b06d7ec).

rriddle accepted this revision.Apr 2 2020, 4:51 PM
rriddle added inline comments.
mlir/examples/standalone/include/Standalone/StandaloneDialect.td
35

nit: I don't think you need the {}, you should be able to just close with ';'.

Kayjukh updated this revision to Diff 254733.Apr 3 2020, 2:26 AM
Kayjukh marked an inline comment as done.

Minor update: remove unneeded braces at the end of the TableGen description for Standalone_Op.

marbre added inline comments.Apr 3 2020, 3:30 AM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

If it is an out-of-tree example, I would avoid using namespace.

59

One could avoid registering all core dialects and passes here, but it might be more useful to change this in a follow-up.

62

A standalone pass is out of scope of this example?

Kayjukh accepted this revision.Apr 3 2020, 5:42 AM
Kayjukh marked 7 inline comments as done.
Kayjukh added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

Is there any particular reason for this? IMO the two using namespace directives make the code easier to read by removing a lot of syntactic overhead. I can understand that in a larger project there might be a potential issue with names colliding but if this is the case, then the maintainer of said project can remove those lines and adapt the code as needed.

59

Yes, agreed, a follow-up may be a better place for this. For now my goal was to have the same behavior as the one described in the documentation when adding your own in-tree dialect to mlir-opt.

62

I was thinking about adding one in a potential follow-up to this patch. As mentioned in the description, this is a bare bones example that could largely be expanded upon. But I think it makes more sense to add additional features one at a time so as not to make a huge patch with everything one would think of in one go. Would this approach make more sense?

marbre added inline comments.Apr 3 2020, 6:26 AM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

It might not be necessary here, but I thought it might be nice to follow the C++ Core Guidelines. The Google C++ Style Guide gives a similar advice: "Do not use using-directives (e.g. using namespace foo)."
Further, Intel did avoid copying using namespace in their ngraph-opt and I didin the forked source for iree-opt.

62

Yes, I am fine with that :)

Kayjukh updated this revision to Diff 254800.Apr 3 2020, 7:33 AM
Kayjukh marked 6 inline comments as done.

Remove the using namespace directives in the standalone-opt source file. This follows standard coding guidelines and code styles of other out-of-tree MLIR projects.

Kayjukh accepted this revision.Apr 3 2020, 7:35 AM
Kayjukh added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

Ok, yes, it makes sense to follow the style guides of other out-of-tree projects.

lattner added inline comments.Apr 3 2020, 9:23 AM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

In my work on an out of tree projects using MLIR, I found it to be pretty impractical to avoid using declarations in general. There is significant syntactic overhead for this, and (IIRC) tblgen output assumes MLIR is in scope in some cases. Similarly, LLVM datatypes like StringRef are pervasive and used by downstream projects.

I have always ended up using a project specific version of things like this file:
https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Support/LLVM.h

but which includes MLIR specific types in addition to LLVM.

Kayjukh marked 2 inline comments as done.Apr 3 2020, 10:02 AM
Kayjukh added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

I think this mostly comes down to personal preferences and coding habits. Personally, I'm not really a fan of enforcing the "no using declarations" rule for every single piece of code, but I can understand that this is a rather controversial opinion in some places.

Maybe we should aim for a middle ground for this patch? The using declarations are still present in dialect-related source files (StandaloneDialect.cpp in this case), but not in the standalone-opt tool source code. This way we don't mess with TableGen regarding name scopes but still avoid using in the main executable.

lattner added inline comments.Apr 3 2020, 12:06 PM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

Yes, I agree. I don't think that perfection is required here, this is an example not a requirement for using MLIR. I'd optimize towards something that lets the code look nice in the example.

People who have local standards that frown upon "using namespace" can and will enforce their local conventions if they base anything on the example.

-Chris

marbre accepted this revision.Apr 3 2020, 1:10 PM
marbre marked an inline comment as done.
marbre added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

Actually, it was just a suggestion, which I thought would be worth to mention, and nothing I tried to enforce :)

Kayjukh marked 3 inline comments as done.Apr 3 2020, 3:05 PM
Kayjukh added inline comments.
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
26

I will leave it like this for now then, and wait until everyone approves the patch.

I think at this point, further issues can be dealt with after this land. I don't think you should expect everyone to accept this, so I would go ahead and check it in. Thanks for doing this!

This revision is now accepted and ready to land.Apr 3 2020, 10:55 PM

Thanks for taking the time to comment on this.

I don't have commit access. Could someone push it to the main repository?

Thanks for taking the time to comment on this.

I don't have commit access. Could someone push it to the main repository?

I'm happy to fix that problem :-). Please read http://llvm.org/docs/DeveloperPolicy.html and send me the requested info, thanks!

This revision was automatically updated to reflect the committed changes.