[PATCH] D80358: [MLIR][InProgress] Add RegionKindInterface

Thu Jun 11 21:57:10 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.
----------------
The "totally ordered" part isn't clear to me here?

================
Comment at: mlir/docs/LangRef.md:34
+within their containing [Region](#regions).  Operations may also
+contain Regions, enabling hiearchical structures to be represented.
+
----------------
Typo "hierarchical"

================
Comment at: mlir/docs/LangRef.md:41
+target-specific instructions, configuration registers, and logic
+gates.  These different concepts are represented by *Operation Types*,
+which can be arbitrarily extended in MLIR.  Verification constraints
----------------
The paragraph starts with "Operations can represent many different concepts", and now the sentence says "These different concepts are represented by *Operation Types*" ; this does not fit.

Maybe you're using "Operation Types" to define what is just "Operation" in MLIR?  Since "Type" is already an IR concept I'd avoid this here.

================
Comment at: mlir/docs/LangRef.md:52
+property.  MLIR includes a 'standard' dialect which defines just such
+strucures.  However, MLIR is intended to be general enough to
+represent other compiler-like data structures, such as Abstract Syntax
----------------
Typo: strucures-> structures

================
Comment at: mlir/docs/LangRef.md:411

-A [block](https://en.wikipedia.org/wiki/Basic_block) is a sequential list of
-operations without control flow (a call or entering an op's region is not
-considered control flow for this purpose) that are executed from top to bottom.
-The last operation in a block is a
-[terminator operation](#terminator-operations), which ends the block.
+A [block] is an ordered list of operations, concluding with a single
+[terminator operation](#terminator-operations).  Blocks commonly
----------------
If you intended to drop the link here, you likely need to remove the surrounding square braces.

================
Comment at: mlir/docs/LangRef.md:414
+represent a compiler basic block
+(https://en.wikipedia.org/wiki/Basic_block) where instructions inside
+the block are executed in the given order and terminator operations
----------------
Square braces around "basic block" to make the link

================
Comment at: mlir/docs/LangRef.md:491
+Regions provide nested control isolation: it is impossible to
+reference (i.e., branch to) a block which is not in the same region as
+the source of the reference (i.e., a terminator operation).
----------------
Not a typography expert, but I'm not used to see a comma after `i.e.` in such context (and for consistency you didn't put one line 420)

================
Comment at: mlir/docs/LangRef.md:538
+
+For dialects associated with 'control flow' semantics, MLIR does not
+restrict how control flow enters or exits a region, or how it flows
----------------
> For dialects associated with 'control flow' semantics,

That does not seems well defined to me: I have read it all but I don't expect an association at the dialect level. Also it would require to specify how do we go from a dialect to a particular region (the dialect from the enclosing operation?)

================
Comment at: mlir/docs/LangRef.md:539
+For dialects associated with 'control flow' semantics, MLIR does not
+restrict how control flow enters or exits a region, or how it flows
+between the blocks in a region. Terminator operations typically
----------------
I thought we were going with CFG semantics for non single-block regions? Did you change your mind here?

That implies that we mandate the first block being the entry block.

================
Comment at: mlir/docs/LangRef.md:546
+control to other basic blocks is determined by the specific dialect
+operations involved.
+
----------------
This is missing the "reachability" aspect: the entry block is only for entry, and any other block is only reachable if it is in the successor list of another operation in the region.

================
Comment at: mlir/docs/LangRef.md:550
+have multiple exit blocks. Any terminator in any block may result in
+'control flow' passing to any other block in the sam region or to the
+enclosing operation. Control flow may also not reach the end of a
----------------
Typo: sam -> same

================
Comment at: mlir/docs/LangRef.md:554
+Concurrent or asynchronous execution of blocks and regions is
+unspecified. The standard dialect leverages this capability
+to define operations with Single-Entry-Multiple-Exit (SEME) regions,
----------------
> Concurrent or asynchronous execution of blocks and regions is unspecified. 

I don't know what you intend to mean here.

================
Comment at: mlir/docs/LangRef.md:563
+the semantics of those regions. Control flow may transfer between
+regions in the enclosing operation. A Region may also enter another
+region within the enclosing operation. If an operation has multiple
----------------
> Control flow may transfer between regions in the enclosing operation

Not directly IMO, a region can only yield to the enclosing operation, the enclosing operation can give control to another region.

================
Comment at: mlir/docs/LangRef.md:564
+regions in the enclosing operation. A Region may also enter another
+region within the enclosing operation. If an operation has multiple
+regions, the semantics of the operation defines into which regions the
----------------
> A Region may also enter another region

What do you mean here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358