[PATCH] D80358: [MLIR] Add RegionKindInterface

Fri Jul 3 21:38:00 PDT 2020

mehdi_amini added inline comments.

================
Comment at: mlir/docs/LangRef.md:33
+are also organized into [Blocks](#blocks) and are totally ordered
+within their containing [Region](#regions).  Operations may also
+contain Regions, enabling hiearchical structures to be represented.
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > stephenneuendorffer wrote:
> > > mehdi_amini wrote:
> > > > stephenneuendorffer wrote:
> > > > > mehdi_amini wrote:
> > > > > > The "totally ordered" part isn't clear to me here?
> > > > > I'm trying to avoid language which implies 'sequential semantics'.  The instructions are ordered (in a list), which can be used in a dialect to model sequential operation.  The only thing that the MLIR core uses this for is to give an iterator ordering to operations, which is reflected in the textual syntax.
> > > > The "totally order" makes me think about https://en.wikipedia.org/wiki/Total_order ; but since your goal is to introduce cycle this seemed contradictory?
> > > Yes, exactly.  The point of this is that the order of instructions in the IR does not need to correspond to the execution order of instructions in a sequential machine.  Even in a graph, the IR still maintains an order, which enables deterministic round-tripping, for instance.
> > > Yes, exactly
> > 
> > I was pointing out something that seems contradictory, so I'm puzzled by your answer here?
> > 
> > > Even in a graph, the IR still maintains an order, which enables deterministic round-tripping, for instance.
> > 
> > Yes but there is still no total order since you have cycles.
> > 
> > 
> > 
> > > The point of this is that the order of instructions in the IR does not need to correspond to the execution order of instructions in a sequential machine. 
> > 
> > OK but the way it is written is still confusing to me. You're also using "Region" as if it is a well defined term while it is the first use in the doc.
> > I'll look for an alternative writing after we settle on the description of multi-blocks region below.
> > 
> > 
> > 
> > Operations are also organized into [Blocks](#blocks) and are totally ordered
> > within their containing [Region](#regions)
> > 
> > 
> > 
> OK.. I see there being several relationships here.  One is the total order of operations in a block and blocks in a region.  There is a separate relationship which is the data dependence relationship, which I don't think is actually a partial order on operations in Graph regions or SSACFG regions.  Certainly not if terminator edges are included.
In the definition of Blocks, we just wrote `A *Block* is an ordered list of operations`, can we just do the same here and avoid "totally ordered"?
Maybe like the following:

```
Operations are also organized into [Blocks](#blocks) as ordered list  (the order may or may not have a semantics important, see [Region Kinds](link)).  Blocks are themselves organized as an ordered list within their containing [Region](#regions).
```

================
Comment at: mlir/docs/LangRef.md:656
+})
+```
+
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > The story about block terminators, successors, etc. isn't clear to me with respect to graph region. Even the relationship between blocks isn't entirely clear here actually. 
> Correct, I haven't defined any semantic requirements about blocks in graph regions.
To me this is like my comment  above about terminator, successor, etc.
Even if it isn't requires, let's make it explicit.

================
Comment at: mlir/docs/LangRef.md:45
+left unspecified, relying on each transformation on operations to be
+designed with the semantics in mind.  Preferably, the semantic
+properties are described abstractly using [Traits](Traits.md) and
----------------
> each transformation on operations to be designed with the semantics in mind

I don't understand what this means here.

================
Comment at: mlir/docs/LangRef.md:211
+Value identifiers are only in scope for the region in which they are
+defined. Argument identifiers in mapping functions are in scope for
+the mapping body.  Particular operations may further limit the valid
----------------
>  Argument identifiers in mapping functions

Not sure what "mapping function" refers to here? It is in the existing doc, but this is the only place it is used. I wonder if it isn't something remaining from before functions were regular Operations?

Is this about block argument? Can you just write "Block argument" instead? 

Edit: I wonder if this is about affine maps?
(we can leave this out of this revision, but we should probably revisit this)

================
Comment at: mlir/docs/LangRef.md:213
+the mapping body.  Particular operations may further limit the valid
+scope of identifiers. For instance, the scope of values in a region
+with [SSA control flow semantics](#control-flow-and-ssacfg-regions) is
----------------
> Particular operations may further limit the valid scope of identifiers

I think this is not specific enough: the scope is only limited by the region kind and I'd rather spell it out this way.
With how you wrote it, one could think that we could have an operation restricting the scope of the value it produces (for example to be block-local).

================
Comment at: mlir/docs/LangRef.md:339
 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
----------------
It isn't clear to me why a module wouldn't be a graph region?
Do we put any meaning to the order of operations in a module?

================
Comment at: mlir/docs/LangRef.md:428
+[terminator operation](#terminator-operations). Blocks commonly
+represent a compiler [basic block]
+(https://en.wikipedia.org/wiki/Basic_block) where instructions inside
----------------
Is "commonly" intended to express that this isn't always the case?
What about spelling out the exact case: "In a SSACFG region, blocks represent ...`

================
Comment at: mlir/docs/LangRef.md:435
+function-like way. Block arguments are bound to values specified by
+the terminator operations, i.e. Branches, which have the block as a
+successor. In regions with [control flow](#control-flow-and-ssacfg-regions),
----------------
> i.e. Branches

Shouldn't this be `e.g. ` instead?

================
Comment at: mlir/docs/LangRef.md:478
 [[more rationale](Rationale/Rationale.md#block-arguments-vs-phi-nodes)].

 ## Regions
----------------
I feel the section here isn't entirely complete with respect to non CFG region with multiple blocks: are these still having terminator? What's the contract on block arguments?

An example would be worthwhile as well.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80358/new/

https://reviews.llvm.org/D80358