This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rkayaith on Sep 10 2022, 11:16 AM.

Details

Summary

This adds a --no-implicit-module option, which disables the insertion
of a top-level builtin.module during parsing. In this mode any op may
be top-level, however it's required that there be exactly one top-level
op in the source.

parseSource{File,String} now support Operation * as the container op
type, which disables the top-level-op-insertion behaviour.

Following patches will add the same option to the other tools as well.

Depends on D133644

Diff Detail

Event Timeline

rkayaith created this revision.Sep 10 2022, 11:16 AM
rkayaith updated this revision to Diff 460108.Sep 14 2022, 8:31 AM

new approach: option to disable all parser magic and allow any op to be top-level

rkayaith updated this revision to Diff 460111.Sep 14 2022, 8:35 AM
rkayaith retitled this revision from [mlir-opt] Allow operations to opt into being top-level operations to [mlir-opt] Support parsing operations other than builtin.module as top-level.
rkayaith edited the summary of this revision. (Show Details)

update description

rkayaith updated this revision to Diff 461407.Sep 19 2022, 4:23 PM
rkayaith edited the summary of this revision. (Show Details)

misc changes

rkayaith published this revision for review.Sep 19 2022, 10:29 PM
rriddle added inline comments.Sep 19 2022, 10:51 PM
mlir/include/mlir/Parser/Parser.h
130–157

Can you just SFINAE the templated parseSourceFile below and handle Operation * separately from other ContainerOpT?

mlir/lib/Parser/Parser.cpp
133–147 ↗(On Diff #461407)

It'd be nice not to have these in the Parser lib, and instead somewhere else. Taking a step back though, is this really worth sharing? as opposed to having this be added only in the necessary places?

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
83

This isn't correct, not all operations can be used with a pass manager. You'll have to check if the op is isolated from above or not.

rkayaith updated this revision to Diff 461634.Sep 20 2022, 11:16 AM

address comments

rkayaith marked 2 inline comments as done.Sep 20 2022, 11:30 AM
rkayaith added inline comments.
mlir/include/mlir/Parser/Parser.h
130–157

I changed detail::constructContainerOpForParserIfNecessary instead of detail::parseSourceFile since it's used by parseSourceString, is this sort of what you had in mind? The new overloads aren't strictly necessary now, but keeping them separate means they get simpler doc strings. I'm also hoping we can eventually deprecate the templated overloads and move the top-level-op-creation logic into the tools instead.

mlir/lib/Parser/Parser.cpp
133–147 ↗(On Diff #461407)

Originally I had this duplicated in each tool, but sharing it helped keep things in sync across tools when iterating. Duplicating it now wouldn't be too bad, though I have a slight preference to sharing since it encourages tools to have consistent parsing behaviour.

Definitely agreed this seems like the wrong place though, I wasn't sure where to put it.

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
83

the pass manager actually checks this already, added a test demonstrating that

rkayaith marked 2 inline comments as done.Sep 20 2022, 11:44 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Parser/Parser.cpp
79 ↗(On Diff #461634)

This description doesn't appear accurate. Instead:
Disable implicit addition of a top-level module op during parsing?

87–88 ↗(On Diff #461634)

I'm not sure this is user-friendly. Instead, asserting with a message will help.

mlir/test/IR/top-level.mlir
4–5

CHECK-NEXT: func.func is enough?

rkayaith updated this revision to Diff 461669.Sep 20 2022, 1:02 PM
rkayaith marked 2 inline comments as done.

improve option description

rkayaith added inline comments.Sep 20 2022, 1:04 PM
mlir/lib/Parser/Parser.cpp
87–88 ↗(On Diff #461634)

This is just following a pattern I saw elsewhere for avoiding a global constructor for the options, e.g. https://github.com/llvm/llvm-project/blob/04df971d65a769d7c03fa69c01a4d302169fdc72/mlir/lib/IR/MLIRContext.cpp#L86-L89

There's an assert below in parseSourceFileForTool though, that'll get hit if a user forgets to call registerToolParserCLOptions

mlir/test/IR/top-level.mlir
4–5

tried that out but I get error: found 'CHECK-NEXT' without previous 'CHECK: line

mehdi_amini added inline comments.Sep 20 2022, 2:35 PM
mlir/lib/Parser/Parser.cpp
85 ↗(On Diff #461669)

Can we avoid handling this as a global?

I rather have the Parser expose an API (ParserConfig) and push the option to CLI tools as needed.

I know MLIRContext does that, but I would not want it done this way today...

rkayaith added inline comments.Sep 20 2022, 4:11 PM
mlir/lib/Parser/Parser.cpp
85 ↗(On Diff #461669)

I was thinking to keep this out of the core parser API, and instead move the global + parseSourceFileForTool somewhere under lib/Tools (or if it's not worth sharing, just duplicate it in all tools like River mentioned ).

But if it's preferable to add it to ParserConfig I can do that instead. I'm not sure how to support the existing parse functions that way though, since they take the top level op as a template arg.

rkayaith updated this revision to Diff 462034.Sep 21 2022, 3:53 PM
rkayaith marked 3 inline comments as done.
rkayaith retitled this revision from [mlir-opt] Support parsing operations other than builtin.module as top-level to [mlir-opt] Support parsing operations other than 'builtin.module' as top-level.
rkayaith edited the summary of this revision. (Show Details)
  • move implicit-module logic into AsmParser, controlled by new option on ParserConfig
  • remove shared cl option

I switched to having the implicit module be controlled by an option on ParserConfig, but now there's two places that are adding implicit top-level ops:

  • in the asm parser, controlled by the ParserConfig, which only supports inserting builtin.module
  • in parseSource{File,String}<ContainerOpT>, which supports inserting other operations

Are there any objections to deprecating the templated overloads in favour of using the ParserConfig option?

mlir/lib/Bytecode/Reader/BytecodeReader.cpp
1419–1421 ↗(On Diff #462034)

it doesn't seem worth supporting this for bytecode, and I'm not sure how I would test it anyways

mlir/lib/Parser/Parser.cpp
85 ↗(On Diff #461669)

Thinking about this some more, if we want to expose this as an option to the python/C bindings then it seems better to just have it as on option on ParserConfig. In the latest diff I switched to that approach and got rid of the global option.

133–147 ↗(On Diff #461407)

I got rid of the shared options in the latest revision

The config option feels like a regression to me. It is now baking builtin.module into places that didn't require it before. I still think this is something that should be left to tools that actually want the implicit module wrapping.

For the templated version of parseSourceFile, I think the ideal end state for those would remove the implicit wrapping and leave that to a different method (just emitting an error if the parsed IR wasn't a single operation of the expected type).

The config option feels like a regression to me. It is now baking builtin.module into places that didn't require it before. I still think this is something that should be left to tools that actually want the implicit module wrapping.

For the templated version of parseSourceFile, I think the ideal end state for those would remove the implicit wrapping and leave that to a different method (just emitting an error if the parsed IR wasn't a single operation of the expected type).

hm okay, then how about adding OwningOpRef<Operation *> parseSourceFileForTool(SourceMgr &sourceMgr, bool insertImplicitModule) to mlir/Support/ToolUtilities.h, and eventually moving the implicit wrapping out of Parser.h and into there?

rkayaith updated this revision to Diff 463284.Sep 27 2022, 11:03 AM
rkayaith edited the summary of this revision. (Show Details)

remove option from ParserConfig, replace with parseSourceFileForTool utility

rriddle accepted this revision.Sep 27 2022, 12:58 PM

LGTM after addressing the comments.

mlir/include/mlir/Parser/Parser.h
208

Do we need new parse methods for these? I think the direction we should likely head in is having parseSourceFile disallow implicit wrapping, and then introduce new parseSourceFileAndWrap methods that wrap the parsed IR if necessary. For example, calling parseSourceFile<ModuleOp> would error if the IR doesn't contain a single ModuleOp, whereas parseSourceFileAndWrap<ModuleOp> would implicitly wrap the IR with a module if necessary (i.e. the current behavior). That would make the behavior during parsing a bit more self-consistent and less surprising.

mlir/include/mlir/Tools/ParseUtilties.h
30–34
This revision is now accepted and ready to land.Sep 27 2022, 12:58 PM
rkayaith added inline comments.Sep 27 2022, 1:21 PM
mlir/include/mlir/Parser/Parser.h
208

What I was planning was to just deprecate parseSourceFile<ContainerOp> entirely and move upstream users to either the new parse methods, or parseSourceFileForTool if they want the wrapping. That way there's a nice warning for downstreams, and the wrapping is kept out of the "core" api here. What you described seems fine too though, I can get rid of these new methods if we want to go with that.

rriddle added inline comments.Sep 27 2022, 5:00 PM
mlir/include/mlir/Parser/Parser.h
208

We shouldn't force all parsing to be through a generic Operation *. Many use cases want to make assumptions about what the top level IR is supposed to be, so we should still accommodate those situations. The behavior we should make more explicit though, is the implicit nesting.

rkayaith updated this revision to Diff 463380.Sep 27 2022, 5:56 PM
rkayaith marked 3 inline comments as done.
rkayaith edited the summary of this revision. (Show Details)

address comments

rkayaith updated this revision to Diff 463381.Sep 27 2022, 6:11 PM

reset author info to make the buildbot happy?

This revision was landed with ongoing or failed builds.Sep 27 2022, 6:14 PM
This revision was automatically updated to reflect the committed changes.