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

Tue Jun 16 00:31:04 PDT 2020

mehdi_amini added a comment.

(Just did another pass)

================
Comment at: mlir/docs/Interfaces.md:209
+
+MLIR provides a suite of interface that provide various functionalities that are
+common across many different operations. Below is a list of some key traits that
----------------
interfaces? (plural)
provides? (`a suite` is the subject I believe)

================
Comment at: mlir/docs/Interfaces.md:211
+common across many different operations. Below is a list of some key traits that
+may be used directly by any dialect. The format of the header for each trait
+section goes as follows:
----------------
I would avoid using "trait" in the two lines above as trait is a specific concept.

================
Comment at: mlir/docs/Interfaces.md:232
+     - `hasDominance(unsigned index)` - Return true if the region with the given index inside this operation requires dominance.
+
----------------
This list seems like something we should generate from ODS?

================
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:
> > 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?

================
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
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > 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.
> > 
> I was trying to get at at the distinction between class Operation and class Op.  To me "Operation Type" is distinct from "Data Types processed by an Operation". I scratched my head for better wording here, but felt like the other things I came up with didn't work either.  Suggestions welcome here if you have a better way to describe this?
What about something like: `These different concepts are expressed by attaching to an Operation definition: traits, interfaces, and more importantly verification constraints`.

================
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
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > 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.
> > 
> I think the discussion was about how to handle unregistered dialects.  In thinking about this further it seems unintuitive to me that some code in a basic block would be legal, but become illegal when an additional basic block is added.  E.g.:  
> 
> Legal.. Might be a 'graph' operation:
>   "unregistered_op" () ({
>   ^bb1:
>     %2 = %1
>     %1 = %2
>   }
> 
> Should be legal?  Might be a graph-like operator using blocks to model something.
>   "unregistered_op" () ({
>   ^bb1:
>     %2 = %1
>     %1 = %2
>   ^bb2:
>     %3 = %2
>   }
> 
> It seems more consistent to me to just let the second case be legal.  Once the dialect is registered, I think it is reasonable to require a dialect to specify whether either case is legal or not.
by doing this you're introducing a mix of "graph-like" and CFG, which seems iffy to me right now.

================
Comment at: mlir/docs/LangRef.md:546
+control to other basic blocks is determined by the specific dialect
+operations involved.
+
----------------
stephenneuendorffer wrote:
> mehdi_amini wrote:
> > 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.
> Hmm... So terminators are required to pass control flow to their successors?  A terminator without successors is not allowed to pass control flow to an arbitrary block in the same region?
That's how we decide "unreachable blocks" today

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

-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
----------------
Nit: remove square braces around `[block]`

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

-A region is a CFG of MLIR [Blocks](#blocks). Regions serve to group semantically
-connected blocks, where the semantics is not imposed by the IR. Instead, the
-containing operation defines the semantics of the regions it contains. Regions
-do not have a name or an address, only the blocks contained in a region do.
-Regions are meaningless outside of the containing entity and have no type or
-attributes.
+A region is an ordered sequence of MLIR [Blocks](#blocks), commonly
+used to represent a Control-Flow Graph (CFG). Regions serve to group
----------------
I'm not sure why we're changing CFG into `ordered sequence` here, this seems under-specified

================
Comment at: mlir/docs/LangRef.md:537
+unspecified and completely specify the behavior of an operation and
+inside the regions of an operation. In MLIR, control flow semantics of
+a region is indicated by `RegionKindInterface::SSACFG`.
----------------
This sentence does not read well to me: "the particular semantics of operations are unspecified and completely specify the behavior of an operation and inside the regions of an operation"

================
Comment at: mlir/docs/LangRef.md:548
+`branch` operation, or back to the containing operation as in a
+`return` operation. Operations without successors can only pass
+control back ot the containing operation. Within these restrictions,
----------------
"Operations without successors" -> "Terminators without successors"

================
Comment at: mlir/docs/LangRef.md:575
+regions, the semantics of the operation defines into which regions the
+control flows and in which order, if any. An operation may also pass
+control flow to multiple contained regions concurrently. An operation
----------------
The sentence that starts with ` If an operation has multiple regions, ...` comes a bit late, the two previous sentences are already on this topic.

================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:51
+      //   return lhs == rhs;
+      // }]
+    >,
----------------
Nit: commented out code to be removed here

================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:57
+  //   return detail::verifyInferredResultTypes($_op);
+  // }];
+}
----------------
(same)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358