This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Move builtin.module LLVM IR translation to before nested operations
ClosedPublic

Authored by skatrak on Aug 18 2023, 6:54 AM.

Details

Summary

This patch moves the call for translating an MLIR module to LLVM IR to the beginning of the translation process. This enables the use of dialect attributes attached to builtin.module operations and the amendOperation() flow to initialize dialect-specific global configuration before translating the contents of the module.

Currently, this patch does not impact the generated IR on its own.

Diff Detail

Event Timeline

skatrak created this revision.Aug 18 2023, 6:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
skatrak requested review of this revision.Aug 18 2023, 6:54 AM

Ping for review! Thank you

kiranchandramohan accepted this revision.Sep 1 2023, 1:28 AM

LGTM. Please wait a couple of days to see if @ftynse @gysit @Dinistro @zero9178 have any comments here.

This revision is now accepted and ready to land.Sep 1 2023, 1:28 AM
zero9178 accepted this revision.Sep 1 2023, 1:45 AM

Makes sense to me. IIRC the dialect conversion framework also does things in pre-order. This now nicely turns the translation into "module -> top-level-ops -> function-bodies" order.

gysit added a comment.Sep 1 2023, 1:53 AM

The change looks good to me!

Would it be possible to add a test, e.g. by implementing the translation interface for the test dialect and then attaching a test dialect attribute to the module?

There have been other revisions that modified the translation order and it may be a good idea to have some testing to ensure follow up commits do not break previous assumptions.

skatrak updated this revision to Diff 555360.Sep 1 2023, 6:42 AM

Add testing infrastructure to allow translating operations from the Test dialect to LLVM IR, and add a simple test to make sure that module attributes are translated before the module body.

skatrak requested review of this revision.Sep 1 2023, 6:43 AM

Thank you all for having a look and for your suggestions! They should be addressed now, but I'd like to request another quick review, since I had to make significant additions in order to test this change using the Test dialect.

zero9178 added inline comments.Sep 1 2023, 7:06 AM
mlir/test/lib/Dialect/Test/CMakeLists.txt
87–88

Having the TestDialect depend on these is problematic I believe as these depend on LLVM proper so will pull in LLVM IR and such as link constructs.
You'd likely see this if building with shared libs.

I think the best way to implement this is to make a separate add_mlir_translation_library just for TestToLLVMIRTranslation that depends on TestDialect, builds`TestToLLVMIRTranslation.cpp` and make mlir-translate link against that if MLIR_INCLUDE_TESTS is true in CMake.

gysit added a comment.Sep 1 2023, 7:11 AM

Thanks for adding the test!

Looks great modulo the packaging in a separate library suggested by @zero9178.

skatrak updated this revision to Diff 555393.Sep 1 2023, 8:38 AM

Split translation of Test dialect to LLVM IR into its own MLIR translation library.

skatrak marked an inline comment as done.Sep 1 2023, 8:43 AM
skatrak added inline comments.
mlir/test/lib/Dialect/Test/CMakeLists.txt
87–88

Thank you for the pointers, I just finished making these changes. In the end, I was able to keep the CMakeLists.txt for mlir-translate unmodified because the add_mlir_translation_library call registers the library into the MLIR_TRANSLATION_LIBS variable that's already used by mlir-translate to find libraries to link to.

zero9178 accepted this revision.Sep 1 2023, 8:46 AM

LGTM

This revision is now accepted and ready to land.Sep 1 2023, 8:46 AM
gysit accepted this revision.Sep 1 2023, 8:47 AM

Thank you for the pointers, I just finished making these changes. In the end, I was able to keep the CMakeLists.txt for mlir-translate unmodified because the add_mlir_translation_library call registers the library into the MLIR_TRANSLATION_LIBS variable that's already used by mlir-translate to find libraries to link to.

Nice!

LGTM

This revision was landed with ongoing or failed builds.Sep 1 2023, 9:09 AM
This revision was automatically updated to reflect the committed changes.
skatrak marked an inline comment as done.

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

No worries! Reverting is the right thing to do :) I believe you might have to use EXCLUDE_FROM_LIBMLIR in your add_mlir_translation_library, just like the test dialect does above. The windows build failure seems unrelated to your change.

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

No worries! Reverting is the right thing to do :) I believe you might have to use EXCLUDE_FROM_LIBMLIR in your add_mlir_translation_library, just like the test dialect does above. The windows build failure seems unrelated to your change.

Thank you, that does seem to be the solution to the problem. I should have read your comment before spending the time reaching the same conclusion! I'll re-land this shortly.

Please always document the commit message when doing reverts including why a revert is happening.

Also instead when reverting reverts, include the original commit message (and mention "re-land commit abc12345" in the description instead), otherwise git log / git blame is harder for everyone.

Please always document the commit message when doing reverts including why a revert is happening.

Also instead when reverting reverts, include the original commit message (and mention "re-land commit abc12345" in the description instead), otherwise git log / git blame is harder for everyone.

Yes, sorry about that. I did get similar feedback right after I re-landed the patch, I'll keep this in mind for next time.