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

Tue Jun 16 19:10:45 PDT 2020

stephenneuendorffer marked 49 inline comments as done.
stephenneuendorffer added a comment.

Update after review changes

================
Comment at: mlir/docs/Interfaces.md:232
+     - `hasDominance(unsigned index)` - Return true if the region with the given index inside this operation requires dominance.
+
----------------
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.

================
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.
----------------
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.

================
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
----------------
rriddle wrote:
> mehdi_amini wrote:
> > 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`.
> I feel the same way as Mehdi. I would rephrase to something like:
> 
> ```
> Operations can be arbitrarily extended in MLIR, and may represent many different concepts ...
> ...
> ```
> 
> I would even consider removing the bit about verification. Feels a bit out of place here.
What is an "Operation Definition" other than traits, interfaces, and verification constraints?  operand and result names? successor names?  I've reworded this section.

================
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
----------------
rriddle wrote:
> mehdi_amini wrote:
> > 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.
> > 
> I don't agree on the principal here. We only have to be conservative if it "could" be something that has valid semantics when registered. For example, we have to be conservative about unregistered operations that "could" be symbol tables, but that doesn't mean we error out if we see any unregistered region holding operation. Being unregistered IMO is just having the capability of being anything legal. If something isn't legal when registered, why should we guard against it when unregistered? 
@rriddle I don't understand whether you are disagreeing with Mehdi or me. 

I think that if ops are unregistered we should be pretty liberal about what should be allowed.  Structually, the only requirement I've imposed on regions of unregistered ops is that all value identifiers must be defined.  In particular, I have not required that regions of unregistered operations must be either 1) a single block or 2) a valid SSACFG region with dominance.  (I think this was suggested in the discussion)

================
Comment at: mlir/docs/LangRef.md:546
+control to other basic blocks is determined by the specific dialect
+operations involved.
+
----------------
rriddle wrote:
> mehdi_amini wrote:
> > 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
> Correct. There is no modeling ATM for indirect branches where you have no idea what the potential destination could be.
I was also thinking about something co-routine like where Blocks could yield to another block chosen by the containing operation.

================
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
----------------
mehdi_amini wrote:
> I'm not sure why we're changing CFG into `ordered sequence` here, this seems under-specified
How so?  The point of this is to show the structures that are important and that are the requirements on those structures, so I'm trying to define a region structurally separate from the CFG semantics.  What would you add to this, other than "it's a CFG?"

================
Comment at: mlir/docs/LangRef.md:545
+Terminator operations ending each block represent control flow by
+explicitly specifying the successor blocks of the block. Control flow
+can only pass to one of the specified successor blocks as in a
----------------
rriddle wrote:
> nit: the potential successor blocks
we don't call them "potential successors' anywhere else.  I think the meaning is clear from the following sentence

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358