[PATCH] D80358: [MLIR] Add RegionKindInterface
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 13 19:02:56 PDT 2020
rriddle accepted this revision.
rriddle marked an inline comment as done.
rriddle added a comment.
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.
================
Comment at: mlir/docs/Interfaces.md:206
+
+
+#### Operation Interface List
----------------
nit: Drop the newline here.
================
Comment at: mlir/docs/Interfaces.md:232
+ - `hasDominance(unsigned index)` - Return true if the region with the given index inside this operation requires dominance.
+
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > This list seems like something we should generate from ODS?
> Certainly! I figure writing a few by hand would lead us to that.
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
================
Comment at: mlir/docs/LangRef.md:29
+MLIR is fundamentally based on a graph-like data structure of nodes,
+called [Operation](#operations), and edges, called *Values*. Each
+Value is the result of exactly one Operation or external Argument, and
----------------
nit: *Operations* to match the *Values* later.
================
Comment at: mlir/docs/LangRef.md:30
+called [Operation](#operations), and edges, called *Values*. Each
+Value is the result of exactly one Operation or external Argument, and
+has a *Value Type* defined by the [type
----------------
nit: I would clarify `Argument` ->`Block Argument` here.
================
Comment at: mlir/docs/LangRef.md:33
+system](#type-system). Operations are contained in [Blocks](#blocks)
+and Blocks are contained in [Region](#regions). Operations are also
+ordered within their containing block and Blocks are ordered in their
----------------
nit: Regions to match the tense of the others.
================
Comment at: mlir/docs/LangRef.md:221
+defined and cannot be accessed or referenced outside of that
+region. Argument identifiers in mapping functions are in scope for the
+mapping body. Particular operations may further limit which
----------------
nit: You may want to clarify here that nested regions count as being within the parent region.
================
Comment at: mlir/docs/LangRef.md:353
An MLIR Module represents a top-level container operation. It contains
-a single region containing a single block which can contain any
-operations. Operations within this region must not implicitly capture
-values defined outside the module. Modules have an optional symbol
-name that can be used to refer to them in operations.
+a single [SSACFG region](#control-flow-and-ssacfg-regions)
+containing a single block which can contain any operations. Operations
----------------
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).
================
Comment at: mlir/docs/LangRef.md:358
+[IsolatedFromAbove](Traits.md#isolatedfromabove). Modules have an
+optional symbol name that can be used to refer to them in operations.
----------------
nit: Could you please you link symbol name to the symbol doc again here?
================
Comment at: mlir/docs/LangRef.md:442
+[terminator operation](#terminator-operations). In [SSACFG
+regions](#control-flow-and-ssacfg-regions), each block represent
+represents a compiler [basic block]
----------------
typo: `represent represents`
================
Comment at: mlir/docs/LangRef.md:449
+Blocks in MLIR take a list of block arguments, notated in a
+function-like way. Block arguments are bound to values specified by
+the terminator operations, e.g. Branches, which have the block as a
----------------
Seems like this only applies to non-entry blocks, as the entry block(s) may have values provided the parent operation.
================
Comment at: mlir/docs/LangRef.md:514
+type or attributes. The first block in the region is a special block
+called the 'entry block'. The entry block cannot be listed as a
+successor of any other block. The syntax for a region is as follows:
----------------
nit: Can you remove the double space before `The`.
================
Comment at: mlir/docs/LangRef.md:532
+same region as the source of the reference, i.e. a terminator
+operation. Similarly, regions provides a natural scoping for value
+visibility: values defined in a region don't escape to the enclosing
----------------
nit; Same here, seems to be a few double space sentences in this paragraph.
================
Comment at: mlir/docs/LangRef.md:543
```mlir
-func @accelerator_compute(i64, i1) -> i64 {
+ "any_op"(%a) ({ // if %a is in-scope in the containing region...
+ // then %a is in-scope here too.
----------------
nit: Remove the indent of this code snippet.
================
Comment at: mlir/docs/LangRef.md:555
+has a parent in the same region, if and only if the parent could use
+the value. A value defined by an argument to a region can always be
+used by any operation deeply contained in the region. A value defined
----------------
nitL Double space here.
================
Comment at: mlir/docs/LangRef.md:569
+executes until the operation is the terminator operation at the end of
+a block, in which case some other operation will execute. The
+determination of the next instruction to execute is the 'passing of
----------------
nit: Double space here.
================
Comment at: mlir/docs/LangRef.md:582
+`return` operation. Terminator operations without successors can only pass
+control back ot the containing operation. Within these restrictions,
+the particular semantics of terminator operations is determined by the
----------------
typo: `ot`.
================
Comment at: mlir/docs/LangRef.md:662
+
+Currently graph regions are arbitrarily limited to a single basic
+block (the entry block), although this may be expanded to allow
----------------
nit: Can you wrap this in a `Context` or `Rationale` section? I think there are other examples in this doc.
================
Comment at: mlir/docs/LangRef.md:674
+order of operations within a block and the order of blocks in a region
+is not semantically meaningful and operations may be freely reordered,
+for instance, by canonicalization. Other kinds of graphs, such as
----------------
nit: I would likely mention that terminator operations cannot be re-ordered.
================
Comment at: mlir/docs/LangRef.md:689
+ %3 = "op2"(%1, %4) : (i32, i32) -> (i32) // OK: %4 allowed here
+^bb2:
+ %4 = "op3"(%1) : (i32) -> (i32)
----------------
Remove this now?
================
Comment at: mlir/include/mlir/IR/Dominance.h:71
-/// A class for computing basic dominance information.
+/// A class for computing basic dominance information. Note that this
+/// class is aware of different types of regions and returns a
----------------
nit: Can you remove the double space sentences in this file?
================
Comment at: mlir/include/mlir/IR/Dominance.h:89
+ /// Return true if operation A properly dominates operation
+ /// B. i.e. if A and B are in the same block and A properly
+ /// dominates B within the block, or if the block that contains A
----------------
nit: `B, i.e.`
================
Comment at: mlir/include/mlir/IR/Dominance.h:97
- /// Return true if operation A dominates operation B.
+ /// Return true if operation A dominates operation B. i.e. if A and
+ /// B are the same operation or A properly dominates B.
----------------
Same here and below: `B, i.e.`
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:18
+#include "mlir/IR/OpDefinition.h"
+#include "mlir/Support/LLVM.h"
+
----------------
I don't think this one is necessary.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:22
+
+// The kinds of regions contained in an operation. SSACFG regions
+// require the SSA-Dominance property to hold. Graph regions do not
----------------
nit: Please use /// here.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:22
+ let description = [{
+ Interface for operations to describe the properties of their regions.
+ }];
----------------
There is a good doc for this in Interfaces.md, can you paste that here?
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:24
+ }];
+
+ let methods = [
----------------
nit: You should be able to set the namespace(via `cppNamespace`) now.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:40
+ /*desc=*/"Return true if the kind of the given region requires the "
+ "SSA-Dominance property",
+ /*retTy=*/"bool",
----------------
nit: the strings are slightly misaligned here.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:30
+ By default, operations of registered dialects contain regions with CFG
+ semantics. This implies that they must satisfy the SSA dominance property.
+ However, other kinds of regions are often used which may not require SSA
----------------
rriddle wrote:
> nit: Remove double space.
Unresolved?
================
Comment at: mlir/lib/IR/Dominance.cpp:38
+ auto kindInterface = dyn_cast<mlir::RegionKindInterface>(op);
+ for (unsigned i = 0; i < op->getNumRegions(); i++) {
+ auto ®ion = op->getRegion(i);
----------------
nit: Cache the end iterator of this loop.
================
Comment at: mlir/lib/IR/Dominance.cpp:39
+ for (unsigned i = 0; i < op->getNumRegions(); i++) {
+ auto ®ion = op->getRegion(i);
// Don't compute dominance if the region is empty.
----------------
nit: Spell out auto here.
================
Comment at: mlir/lib/IR/Dominance.cpp:231
+ return a->isBeforeInBlock(b);
+ } else {
+ return true;
----------------
nit: Drop else after return.
================
Comment at: mlir/lib/IR/Dominance.cpp:255
+ Operation *ancestor = aRegion->getParentOp();
+ auto kindInterface = dyn_cast<mlir::RegionKindInterface>(ancestor);
+ bool hasSSADominance =
----------------
nit: Please drop the uses of `mlir::` in this file.
================
Comment at: mlir/lib/IR/Dominance.cpp:257
+ bool hasSSADominance =
+ ancestor->isRegistered() &&
+ (!kindInterface || kindInterface.hasSSADominance(aRegionNum));
----------------
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));
}
```
================
Comment at: mlir/lib/IR/Dominance.cpp:303
+ return b->isBeforeInBlock(a);
+ } else {
+ return true;
----------------
Same here, drop else after return.
================
Comment at: mlir/lib/IR/Verifier.cpp:188
// Verify that all child regions are ok.
- for (auto ®ion : op.getRegions())
+ for (unsigned i = 0; i < op.getNumRegions(); i++) {
+ auto ®ion = op.getRegion(i);
----------------
nit: Cache the end iterator here.
================
Comment at: mlir/lib/IR/Verifier.cpp:189
+ for (unsigned i = 0; i < op.getNumRegions(); i++) {
+ auto ®ion = op.getRegion(i);
+ // Check that Graph Regions only have a single basic block. This is
----------------
nit: Spell out auto here.
================
Comment at: mlir/lib/IR/Verifier.cpp:203
+ if (std::next(region.begin()) != region.end())
+ return op.emitOpError("expects region #")
+ << i << " to have 0 or 1 blocks";
----------------
nit: `expected graph region`?
================
Comment at: mlir/lib/IR/Verifier.cpp:270
+ // a region which doesn't respect dominance.
+ for (auto &op : block)
+ if (failed(verifyDominanceOfContainedRegions(op)))
----------------
nit: Spell out auto here.
================
Comment at: mlir/lib/IR/Verifier.cpp:280
+OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) {
+ for (unsigned i = 0; i < op.getNumRegions(); i++) {
+ auto ®ion = op.getRegion(i);
----------------
nit: Cache the end iterator here, and can you inline the `region` variable below into its use?
================
Comment at: mlir/lib/Transforms/CSE.cpp:174
+ // If the region does not have dominanceInfo, then skip it. This is
+ // a temporary step. Probably, we just need to apply a different
----------------
Can you reword the second part of this into a TODO:?
================
Comment at: mlir/lib/Transforms/CSE.cpp:175
+ // If the region does not have dominanceInfo, then skip it. This is
+ // a temporary step. Probably, we just need to apply a different
+ // traversal method.
----------------
nit: Drop the double space here.
================
Comment at: mlir/lib/Transforms/CSE.cpp:177
+ // traversal method.
+ if (!domInfo.hasDominanceInfo(®ion)) {
+ return;
----------------
nit: Drop trivial braces.
================
Comment at: mlir/test/IR/parser.mlir:1248
func @unreachable_dominance_violation_ok() -> i1 {
- %c = constant false // CHECK: [[VAL:%.*]] = constant false
- return %c : i1 // CHECK: return [[VAL]] : i1
-^bb1: // CHECK: ^bb1: // no predecessors
+ %c = constant false // CHECK: [[VAL:%.*]] = constant false
+ return %c : i1 // CHECK: return [[VAL]] : i1
----------------
Can you move the CHECK lines to non-IR lines to match the other tests? Ends up being much cleaner IMO.
================
Comment at: mlir/test/IR/traits.mlir:406
+ }
+ return // CHECK: return
+} // CHECK: }
----------------
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.
================
Comment at: mlir/test/IR/traits.mlir:415
+ test.graph_region { // CHECK: test.graph_region {
+ // %1 is not dominated by its definition.
+ %2:3 = "bar"(%1) : (i64) -> (i1,i1,i1) // CHECK: [[VAL2:%.*]]:3 = "bar"([[VAL3]]) : (i64) -> (i1, i1, i1)
----------------
nit: This comment seems wrong.
================
Comment at: mlir/test/IR/traits.mlir:428
+ test.graph_region {
+// expected-error @+1 {{operand #0 does not dominate this use}}
+ %2:3 = "bar"(%1) : (i64) -> (i1,i1,i1)
----------------
nit: Can you indent these expected lines? Same below.
================
Comment at: mlir/test/IR/traits.mlir:442
+ "test.ssacfg_region"() ({ // CHECK: "test.ssacfg_region"() ( {
+ %2:3 = "bar"(%1) : (i64) -> (i1,i1,i1) // CHECK: %1:3 = "bar"(%0) : (i64) -> (i1, i1, i1)
+ }) : () -> () // CHECK: }) : () -> ()
----------------
nit: Please remove the explicit SSA value names, and please remove the unnecessary bits of this test.
================
Comment at: mlir/test/IR/traits.mlir:505
+func @graph_region_cant_have_blocks() {
+test.graph_region {
+// expected-error at -1 {{'test.graph_region' op expects region #0 to have 0 or 1 blocks}}
----------------
nit: Indent the graph region.
================
Comment at: mlir/test/lib/Dialect/Test/TestDialect.cpp:227
+
+RegionKind mlir::DominanceFreeScopeOp::getRegionKind(unsigned index) {
+ return RegionKind::GRAPH;
----------------
rriddle wrote:
> Please remove the `mlir::` from each of these.
Unresolved here?
================
Comment at: mlir/test/lib/Dialect/Test/TestOps.td:1196
+ let description = [{
+ Test op that defines an SSACFG region.
+ }];
----------------
nit: You could use a "" for the descriptions as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80358/new/
https://reviews.llvm.org/D80358
More information about the llvm-commits
mailing list