Page MenuHomePhabricator

[MLIR] Add RegionKindInterface
ClosedPublic

Authored by stephenneuendorffer on May 20 2020, 10:31 PM.

Details

Summary

Some dialects have semantics which is not well represented by common
SSA structures with dominance constraints. This patch allows
operations to declare the 'kind' of their contained regions.
Currently, two kinds are allowed: "SSACFG" and "Graph". The only
difference between them at the moment is that SSACFG regions are
required to have dominance, while Graph regions are not required to
have dominance. The intention is that this Interface would be
generated by ODS for existing operations, although this has not yet
been implemented. Presumably, if someone were interested in code
generation, we might also have a "CFG" dialect, which defines control
flow, but does not require SSA.

The new behavior is mostly identical to the previous behavior, since
registered operations without a RegionKindInterface are assumed to
contain SSACFG regions. However, the behavior has changed for
unregistered operations. Previously, these were checked for
dominance, however the new behavior allows dominance violations, in
order to allow the processing of unregistered dialects with Graph
regions. One implication of this is that regions in unregistered
operations with more than one op are no longer CSE'd (since it
requires dominance info).

I've also reorganized the LangRef documentation to remove assertions
about "sequential execution", "SSA Values", and "Dominance". Instead,
the core IR is simply "ordered" (i.e. totally ordered) and consists of
"Values". I've also clarified some things about how control flow
passes between blocks in an SSACFG region. Control Flow must enter a
region at the entry block and follow terminator operation successors
or be returned to the containing op. Graph regions do not define a
notion of control flow.

see discussion here:
https://llvm.discourse.group/t/rfc-allowing-dialects-to-relax-the-ssa-dominance-condition/833/53

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mehdi_amini added inline comments.Jul 3 2020, 9:18 PM
mlir/docs/LangRef.md
684

To me this is like my comment above about terminator, successor, etc.
Even if it isn't requires, let's make it explicit.

stephenneuendorffer marked 18 inline comments as done.Jul 6 2020, 9:58 AM
stephenneuendorffer added inline comments.
mlir/docs/LangRef.md
80

I've rewritten this section.

219

Yes, I believe this is talking about affine maps.

221

You're right, that could be misleading. I've rewritten it.

351–357

I suppose it could be, although it's somewhat of a degenerate kind of region anyway, since modules don't define any values and don't contain blocks. Maybe this should be another kind of region?

496

I added an additional note about the Graph Region rationale, but I'm really not sure how to provide an example here of something that I don't have a good example of :). Let's chat offline about this.

stephenneuendorffer marked 4 inline comments as done.
mehdi_amini added inline comments.Jul 6 2020, 10:01 PM
mlir/docs/LangRef.md
683

The indentation does not seem right in this example?
The reuse of %2 does not help readability either, just like %4 before %2

stephenneuendorffer marked 2 inline comments as done.
rriddle accepted this revision.Jul 13 2020, 7:02 PM
rriddle marked an inline comment as done.

Thanks for all of the work on this! Mostly nits, so I think this looks good enough to land for now after resolving. We can iterate on the other necessary aspects afterwards.

mlir/docs/Interfaces.md
206

nit: Drop the newline here.

232

ODS can already generate the documentation. I'll take a look at hooking it into here after this lands.

https://github.com/llvm/llvm-project/blob/bf74c3838904b2df2a0f31bc457af1bdb811953f/mlir/tools/mlir-tblgen/OpInterfacesGen.cpp#L414

mlir/docs/LangRef.md
64

nit: *Operations* to match the *Values* later.

65

nit: I would clarify Argument ->Block Argument here.

68

nit: Regions to match the tense of the others.

220

nit: You may want to clarify here that nested regions count as being within the parent region.

351–357

Should a module be an SSA CFG region? It could very well be a graph region.

(This is not meant to block anything, but something to consider for the future).

356

nit: Could you please you link symbol name to the symbol doc again here?

434

typo: represent represents

434

Seems like this only applies to non-entry blocks, as the entry block(s) may have values provided the parent operation.

513

nit: Can you remove the double space before The.

548

nit; Same here, seems to be a few double space sentences in this paragraph.

601

nit: Remove the indent of this code snippet.

613

nitL Double space here.

627

nit: Double space here.

640

typo: ot.

660

nit: Can you wrap this in a Context or Rationale section? I think there are other examples in this doc.

672

nit: I would likely mention that terminator operations cannot be re-ordered.

687

Remove this now?

mlir/include/mlir/IR/Dominance.h
71–73

nit: Can you remove the double space sentences in this file?

87

nit: B, i.e.

94–95

Same here and below: B, i.e.

mlir/include/mlir/IR/RegionKindInterface.h
19

I don't think this one is necessary.

23

nit: Please use /// here.

mlir/include/mlir/IR/RegionKindInterface.td
23

There is a good doc for this in Interfaces.md, can you paste that here?

25

nit: You should be able to set the namespace(via cppNamespace) now.

31

Unresolved?

41

nit: the strings are slightly misaligned here.

mlir/lib/IR/Dominance.cpp
46

nit: Cache the end iterator of this loop.

47

nit: Spell out auto here.

240

nit: Drop else after return.

262

nit: Please drop the uses of mlir:: in this file.

264

nit: Can you define a static helper for this in this file?

static bool hasSSADominance(Operation *op, RegionKindInterface interface, unsigned regionNum) {
    return ancestor->isRegistered() &&
          (!kindInterface || kindInterface.hasSSADominance(aRegionNum));
}
308

Same here, drop else after return.

mlir/lib/IR/Verifier.cpp
187–205

nit: Cache the end iterator here.

188

nit: Spell out auto here.

202

nit: expected graph region?

280

nit: Cache the end iterator here, and can you inline the region variable below into its use?

282

nit: Spell out auto here.

mlir/lib/Transforms/CSE.cpp
174

Can you reword the second part of this into a TODO:?

175

nit: Drop the double space here.

177

nit: Drop trivial braces.

mlir/test/IR/parser.mlir
1249–1262

Can you move the CHECK lines to non-IR lines to match the other tests? Ends up being much cleaner IMO.

mlir/test/IR/traits.mlir
406

nit: Please remove the CHECK lines that don't relate to what you are checking. This applies to the other tests as well. e.g. You don't really need to check return or the braces here.

415

nit: This comment seems wrong.

428

nit: Can you indent these expected lines? Same below.

442

nit: Please remove the explicit SSA value names, and please remove the unnecessary bits of this test.

505

nit: Indent the graph region.

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

Unresolved here?

mlir/test/lib/Dialect/Test/TestOps.td
1196

nit: You could use a "" for the descriptions as well.

stephenneuendorffer marked 56 inline comments as done.Jul 14 2020, 3:10 PM

If there are no further comments, I'd like to commit this. @bondhugula @mehdi_amini Do you want to accept?

Note that there's a small amount of new text under "### Regions" where I tried to capture the understanding about how Region Arguments are shared with the Block Arguments of the entry block, but that operation arguments/return values are explicitly decoupled from region arguments and terminators. Also terminator arguments do not need to match the block arguments of their successor blocks. All of this is under the control of specific operations and may be arbitrarily defined.

mlir/docs/LangRef.md
647

I've reworded this.

683

Actually, there were a few other minor problems with this testcase, including the fact that it missed out on the fact that:

%1 = op(%1)

should be valid, along with:

%1 = op() { otherop(%1) }

I also turned this into another testcase.

stephenneuendorffer edited the summary of this revision. (Show Details)
stephenneuendorffer marked an inline comment as done.
bondhugula added inline comments.Jul 15 2020, 11:27 AM
mlir/include/mlir/IR/Dominance.h
86–92

Reflow: I think you aren't using the whole width.

mlir/lib/IR/Dominance.cpp
27

These should be ///

43

Nit: This should be //.

262–264

Nit: reflow to use width.

mlir/lib/IR/Verifier.cpp
66

Nit: terminate comment with period.

Quick question: how do we know in printed IR if something is a graph region or an SSA one?

mlir/docs/LangRef.md
65

a Block Argument?

77–107

Nit: Reflow to use width.

stephenneuendorffer marked 8 inline comments as done.Jul 15 2020, 12:40 PM

Quick question: how do we know in printed IR if something is a graph region or an SSA one?

You don't. Syntactically, there is no distinction for unregistered ops. Unregistered ops are handled conservatively as graph regions, so they are allowed to violate SSA dominance. Passes that require dominance-order traversal (i.e. CSE) won't operate on regions inside unregistered ops.

mlir/docs/LangRef.md
65

I parse it as 'exactly one (Operation or Block Argument)', so I think it makes sense as is?

stephenneuendorffer marked an inline comment as done.
This revision was not accepted when it landed; it landed in state Needs Review.Jul 15 2020, 2:32 PM
This revision was automatically updated to reflect the committed changes.