[PATCH] D80358: [MLIR] Add RegionKindInterface

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer marked an inline comment as done.
stephenneuendorffer added inline comments.


================
Comment at: mlir/docs/LangRef.md:495
+type or attributes. The first block in the region cannot be a
+successor of any other block. The syntax for the region is as follows:
 
----------------
mehdi_amini wrote:
> You define a notion of successor here, does it generalize to graph region?
Currently we keep around blocks in a graph region after transforming from an SSACFG region.  The blocks aren't semantically meaningful (and can be eliminated in our current model without changing the semantics), but are at least useful bookkeeping.  


================
Comment at: mlir/docs/LangRef.md:539
+executes until the operation is the terminator operation at the end of
+a block, in which case some other operation will execute.  The
+determination of the next instruction to execute is the 'passing of
----------------
mehdi_amini wrote:
> > in which case some other operation will execute
> 
> That does not read well to me right now.
I agree it's a little vague, but it's trying to bridge into the next section which talks about control flow and terminators.  Do you have a suggestion?


================
Comment at: mlir/docs/LangRef.md:595
 
-This can be further restricted using the custom verifier associated with the
-enclosing operation, for example, disallowing references to values defined
-outside the region completely.
-
-### Control Flow
-
-Regions are Single-Entry-Multiple-Exit (SEME). This means that control can only
-flow into the first block of the region, but can flow out of the region at the
-end of any of the contained blocks (This behavior is similar to that of a
-function body in most programming languages). A terminator of a block within a
-region may transfer the control flow to another block in this region, or return
-it to the immediately enclosing op. The semantics of the enclosing op defines
-where the control flow is transmitted next. It may, for example, enter a region
-of the same op, including the same region that returned the control flow.
-
-The enclosing operation determines the way in which control is transmitted into
-the entry block of a Region. The successor to a Region’s exit points may not
-necessarily exist: for example a call to a function that does not return.
-Concurrent or asynchronous execution of Regions is unspecified. Operations may
-define specific rules of execution, e.g. sequential loops or switch cases.
-
-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 control flows and in which order, if any. An operation may
-transmit control into Regions that were specified in other operations, in
-particular those that defined the values the given operation uses. Thus, such
-operations can be treated opaquely in the enclosing control flow graph,
-providing a level of control flow isolation similar to that of the call
-operation.
+#### Operations with Multiple Regions
+
----------------
mehdi_amini wrote:
> It is interesting that this ends up nested under the SSACFG region case
It's all about control flow with multiple regions.  Graph regions don't have a notion of control flow.


================
Comment at: mlir/docs/LangRef.md:656
+})
+```
+
----------------
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.


================
Comment at: mlir/include/mlir/IR/Dominance.h:63
   /// Return true if the specified block is reachable from the entry
   /// block of its region.
   bool isReachableFromEntry(Block *a) const;
----------------
mehdi_amini wrote:
> The concept of entry block is no longer a guarantee
There are some aspects of the interface of this class which are awkward.  If regions define their own concept of dominance, then there probably needs to be an abstract interface exposing different implementations of the 'local' concept of dominance.  Currently, the two implementations are somewhat awkwardly tangled together along with the hierarchy traversing code.  I see that there should be some additional refactoring of this code which might push the local concept of dominance into RegionKindInterface, but I'd like to get the concepts agreed to first.

Another example is that getRootNode() has to be guarded by hasDominanceInfo().  getRootNode() isn't really appropriate for regions without an entry block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358