Page MenuHomePhabricator

Define a `NoTerminator` traits that allows operations with a single block region to not provide a terminator
ClosedPublic

Authored by mehdi_amini on Mar 11 2021, 4:19 PM.

Details

Summary

In particular for Graph Regions, the terminator needs is just a
historical artifact of the generalization of MLIR from CFG region.
Operations like Module don't need a terminator, and before Module
migrated to be an operation with region there wasn't any needed.

To validate the feature, the ModuleOp is migrated to use this trait and
the ModuleTerminator operation is deleted.

This patch is likely to break clients, if you're in this case:

  • you may iterate on a ModuleOp with getBody()->without_terminator(), the solution is simple: just remove the ->without_terminator!
  • you created a builder with Builder::atBlockTerminator(module_body), just use Builder::atBlockEnd(module_body) instead.
  • you were handling ModuleTerminator: it isn't needed anymore.
  • for generic code, a Block::mayNotHaveTerminator() may be used.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mehdi_amini requested review of this revision.Mar 11 2021, 4:19 PM

Minor cleanup

Fix the examples

Update python bindings

Rebase and add support for unregistered operation

rriddle added inline comments.Mar 16 2021, 11:38 AM
mlir/include/mlir/IR/Block.h
229 ↗(On Diff #330782)

When would this be used outside of the verifier?

mlir/include/mlir/IR/OpDefinition.h
658

Can you verify this?

782

llvm::hasSingleElement

786–790

Isn't this already verified by the verifier?

843–844

This looks weird, is this necessary?

mlir/include/mlir/IR/OpImplementation.h
95

Can you document the rest of the parameters as well while you are here? (I wonder if we should also consider a flag builder here at some point, we now have 3 bool flags)

mlir/include/mlir/IR/RegionKindInterface.h
32
mlir/include/mlir/Parser.h
41–43

Can you check this clang-tidy message?

mlir/lib/IR/AsmPrinter.cpp
2550

Can you define a local variable for this?

mlir/lib/IR/SymbolTable.cpp
167

nit: Add braces to this if/else now, the body is quite large.

300

Can you cache the region to avoid computing it multiple times?

mlir/tools/mlir-tblgen/OpFormatGen.cpp
456

Drop trivial braces here.

mehdi_amini marked 10 inline comments as done.Mar 17 2021, 9:04 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/Block.h
229 ↗(On Diff #330782)

I am not sure, but any pass that needs to be conservative in the same way.
I suspect it is similar to mightHaveTrait<...>()

mlir/include/mlir/IR/OpDefinition.h
658

I would need to include the RegionKind header in here, should I do that?

Also Chris mentioned on Discourse some non-graph region which aren't intended to return values and could also skip the terminator, what about removing the comment here?
(my original implementation was relying on graph region in some places but that was before I made it a "NoTerminator" trait).

782

(note this is just code motion, but still I fixed it)

786–790

That's a good point! (note this is code motion only again).

Unfortunately this is needed because the verifier won't kick in for nested block before verifying the enclosing operation itself: https://github.com/llvm/llvm-project/blob/main/mlir/test/IR/traits.mlir#L301-L306

843–844

indeed... (removed)

mlir/include/mlir/Parser.h
41–43

Yes, I'll need your magic to restore the version on the left, but without using ImplicitTerminatorOpT. The existing assert is actually failing to do what it is suppose to because if an op does not implement the SingleBlockImplicitTerminator it does not have the ImplicitTerminatorOpT in the first place!

mehdi_amini marked 4 inline comments as done.
mehdi_amini edited the summary of this revision. (Show Details)

Addressing comments (most)

Fix the trait detection for SingleBlockImplicitTerminatorOp

rriddle accepted this revision.Mar 19 2021, 2:52 PM
rriddle added inline comments.
mlir/include/mlir/IR/OpDefinition.h
842

nit: Block *body or getBody()->getOperations()...?

mlir/lib/IR/SymbolTable.cpp
300

Why not just region?

mlir/test/lib/Dialect/Test/TestDialect.cpp
38

Why is this in TestDialect.cpp?

This revision is now accepted and ready to land.Mar 19 2021, 2:52 PM

Did you make sure that the docs are up-to-date with this?

mehdi_amini added inline comments.Mar 19 2021, 2:55 PM
mlir/lib/IR/SymbolTable.cpp
300

Sure, renamed.

Do we verify that SymbolTable operations are single region by the way?
It's annoying I was planning to have an op with two regions and each would be a symbol table. I guess that's a problem for nested symbol refs...

rriddle added inline comments.Mar 19 2021, 3:00 PM
mlir/lib/IR/SymbolTable.cpp
300

Yes, it is verified. For multi-region symbol tables, it is theoretically possible but we would need to:
a) invent some special new syntax
b) A lot of the code in this file would need to be rewritten to not anchor on the symbol table operation itself.

Did you make sure that the docs are up-to-date with this?

Excellent point, I haven't yet@

mlir/include/mlir/IR/OpDefinition.h
842

getBody()->getOperations() is better!

mlir/test/lib/Dialect/Test/TestDialect.cpp
38

I needed an op which defined the traits to be able to test that.
Otherwise we don't instantiate this anywhere in the codebase.

mehdi_amini marked 5 inline comments as done.Mar 19 2021, 3:10 PM

Address comments (doc not done yet)

Update LangRef

Minor TableGen rename for the trait list

Update Traits doc

rriddle accepted this revision.Mar 19 2021, 3:52 PM

LGTM

Harbormaster completed remote builds in B94799: Diff 332015.
This revision was landed with ongoing or failed builds.Wed, Mar 24, 8:59 PM
This revision was automatically updated to reflect the committed changes.
kiranchandramohan added inline comments.
mlir/lib/IR/Block.cpp
303

This function is not used currently and there is an unused warning. Is there a plan to use it in future? There is an identical function in the verifier in this patch.

stella.stamenova added inline comments.
mlir/lib/IR/Block.cpp
303

+1, let's keep to one function instead of duplicating code in multiple places

mehdi_amini added inline comments.Thu, Mar 25, 11:38 AM
mlir/lib/IR/Block.cpp
303

That was an oversight, it was here initially and I meant to move it to the Verifier and instead I duplicated it.

Removed in fcdf142ed59c !