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

Mon Jun 15 23:04:24 PDT 2020

stephenneuendorffer added a comment.

Thanks for the careful reviews.  It resulted in many updates in the documentation.

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

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

================
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
----------------
mehdi_amini wrote:
> > 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?)
oops. This should be talking about operations with SSACFG regions.  I've clarified this.

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

================
Comment at: mlir/docs/LangRef.md:546
+control to other basic blocks is determined by the specific dialect
+operations involved.
+
----------------
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?

================
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,
----------------
mehdi_amini wrote:
> > Concurrent or asynchronous execution of blocks and regions is unspecified. 
> 
> I don't know what you intend to mean here.
This ended up out of context, I think.  In this section, I think it's appropriate to say only that regions in an operation can execute concurrently.  Blocks *are* actually expected to execute sequentially, I think.

================
Comment at: mlir/lib/IR/Verifier.cpp:64-65
+  LogicalResult verifyDominance(Region &region, bool inDominanceScope);
   LogicalResult verifyDominance(Operation &op);
+  LogicalResult verifyDominanceOfContainedRegions(Operation &op);

----------------
bondhugula wrote:
> This needs a comment - otherwise it may not be clear to new readers whether the former check is a strict superset of the latter.
I removed verifyDominance(Operation) and better commented the others.

================
Comment at: mlir/lib/IR/Verifier.cpp:225

-LogicalResult OperationVerifier::verifyDominance(Region &region) {
+LogicalResult OperationVerifier::verifyDominance(Region &region, bool inDominanceScope) {
   // Verify the dominance of each of the held operations.
----------------
bondhugula wrote:
> inDominanceScope sounds odd. Switch to isDominanceFreeScope and negate?
I think from the perspective of this code "inDominanceScope" is more meaningful, since we do fewer checks when inDominanceScope=false.  "DominanceFreeScope" is now only present in the tests (although it probably make sense to rename them at this point)

================
Comment at: mlir/lib/IR/Verifier.cpp:267
+LogicalResult OperationVerifier::verifyDominanceOfContainedRegions(Operation &op) {
+  bool inDominanceScope = !op.hasTrait<OpTrait::DominanceFreeScope>();
   for (auto &region : op.getRegions())
----------------
bondhugula wrote:
> Perhaps drop the !, and nDominanceScope -> isDominanceFreeScope?
I changed these all to "hasDominance"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358