Page MenuHomePhabricator

[mlir-translate] Support parsing operations other than 'builtin.module' as top-level
ClosedPublic

Authored by rkayaith on Sep 19 2022, 4:26 PM.

Details

Summary

This adds a '--no-implicit-module' option, which disables the insertion
of a top-level 'builtin.module' during parsing.

The translation APIs are also updated to take/return 'Operation*'
instead of 'ModuleOp', to allow other operation types to be used. To
simplify translations which are restricted to specific operation types,
'TranslateFromMLIRRegistration' has an overload which performs the
necessary cast and error checking.

Diff Detail

Event Timeline

rkayaith created this revision.Sep 19 2022, 4:26 PM
rkayaith added inline comments.Sep 19 2022, 10:23 PM
mlir/test/Target/SPIRV/module.mlir
1 ↗(On Diff #461409)

I only updated one test, but actually all the spirv translate tests work with --no-implicit-module. The translation code could be simplified a bit if this was the only mode that needed to be supported, since the IR walks to find a spv.module could be removed

rkayaith published this revision for review.Sep 19 2022, 10:29 PM
rkayaith updated this revision to Diff 463371.Sep 27 2022, 4:55 PM
rkayaith retitled this revision from [mlir-translate] Support parsing operations other than builtin.module as top-level to [mlir-translate] Support parsing operations other than 'builtin.module' as top-level.

rebase

rkayaith updated this revision to Diff 463389.Sep 27 2022, 6:36 PM

fix build bot?

rkayaith added inline comments.
mlir/lib/Tools/mlir-translate/Translation.cpp
106–109

Putting the cl option here doesn't seem great, but it's hard to plumb it to the parse call otherwise. Open to other ideas.

friendly ping :)

mehdi_amini added inline comments.Oct 11 2022, 11:21 PM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

I think if would be more friendly if we could register these with a type safe trampoline. Something that automatically dyn_cast the operation* to the Op expected by the call back and print an error message and fails before calling the translation call back in case of mismatch.

rriddle added inline comments.Oct 12 2022, 12:01 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1192–1196
rkayaith updated this revision to Diff 467169.Oct 12 2022, 8:56 AM
rkayaith marked an inline comment as done.
rkayaith edited the summary of this revision. (Show Details)

address comment

rkayaith added inline comments.Oct 12 2022, 8:57 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

I can give this a try but I don't think any of the upstream translations would actually use this at the moment, unless I changed the spirv translations to only work with --no-implicit-module.

mehdi_amini added inline comments.Oct 12 2022, 9:54 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

I'm not sure you got my point here, let me try to reformulate: we should make it so that the current translation registration don't need to be updated with their current callback.

They are passing a lambda that is [](ModuleOp module, raw_ostream &output) { and they should be able to continue to do that.

The behavior of the system is that it should take a function with the signature LogicalResult(T module, raw_ostream &output) and then check after parsing that the parsed operation is actually T or error out.

mehdi_amini added inline comments.Oct 12 2022, 9:57 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

It reminds of the logic we use with walk([] (T op) {...}) where we check that we match T before calling the callback: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/Visitors.h#L227-L229

rkayaith added inline comments.Oct 12 2022, 11:01 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

Sure, what I meant though is that I don't see why any of the upstream translations would want to continue taking a ModuleOp:

The api you proposed makes sense to me, but is there an issue if it ends up unused upstream?

mehdi_amini added inline comments.Oct 12 2022, 11:46 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

Can we default to --no-implicit-module for translations?

rriddle added inline comments.Oct 12 2022, 11:58 AM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

Could we just remove it altogether? Translations are way less common than passes.

rkayaith added inline comments.Oct 12 2022, 12:30 PM
mlir/include/mlir/Tools/mlir-translate/Translation.h
49

if I flip the default I only get 24 failures (out of 81). but I think it would be confusing if the default doesn't match mlir-opt.

rkayaith updated this revision to Diff 467277.Oct 12 2022, 3:24 PM

add a TranslateFromMLIRRegistration constructor that handles casting to concrete op type

mlir/include/mlir/Tools/mlir-translate/Translation.h
49

I added a TranslateFromMLIRRegistration constructor that handles the casting, and for now I switched the spirv translation back to only working for builtin.module so that something actually uses it. and a patch here to switch it to work on spirv.module instead if we want that: https://reviews.llvm.org/D135819

rriddle accepted this revision.Oct 20 2022, 6:07 PM
rriddle added inline comments.
mlir/include/mlir/Tools/mlir-translate/Translation.h
86–94
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
1168–1169

We should likely special case conversions from OwningOpRef<FooOp> to OwningOpRef<Operation *>.

This revision is now accepted and ready to land.Oct 20 2022, 6:07 PM
rkayaith added inline comments.Oct 20 2022, 7:36 PM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
1168–1169

so something like this on OwningOpRef?

operator OwningOpRef<Operation *>() && { return release().getOperation(); }
rkayaith updated this revision to Diff 469716.Oct 21 2022, 12:34 PM
rkayaith marked 5 inline comments as done.
rkayaith edited the summary of this revision. (Show Details)

update description, use OwningOpRef conversion, address comment

This revision was landed with ongoing or failed builds.Oct 21 2022, 12:54 PM
This revision was automatically updated to reflect the committed changes.
Peiming added inline comments.
mlir/lib/Tools/mlir-translate/Translation.cpp
106–109

This seems break some customized mlir-opt tools (which calls MlirOptMain) and need to register the translation here as they both register the cl option, leading to Option 'no-implicit-module' registered more than once!

mehdi_amini added inline comments.Oct 22 2022, 5:18 PM
mlir/lib/Tools/mlir-translate/Translation.cpp
106–109

Yes a static isn't great here, that said it's not clear to me why you'd have global translate registrations in a mlir-opt tool?

Peiming added inline comments.Oct 22 2022, 9:59 PM
mlir/lib/Tools/mlir-translate/Translation.cpp
106–109

I will offer you more details offline :-)

rkayaith added inline comments.Oct 23 2022, 12:44 PM
mlir/lib/Tools/mlir-translate/Translation.cpp
106–109

I think D136561 should let you work around this in your tool by skipping the option registration

mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp:67:30: error: use 'template' keyword to treat 'dyn_cast_or_null' as a dependent template name
                            .dyn_cast_or_null<arith::FastMathFlagsAttr>();
                             ^
                             template

The bot is broken, are you on it already? https://lab.llvm.org/buildbot/#/builders/61/builds/34355

i think you meant to comment on D126305