[PATCH] D80358: [MLIR] Add RegionKindInterface

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer marked 18 inline comments as done.
stephenneuendorffer added inline comments.


================
Comment at: mlir/docs/LangRef.md:45
+left unspecified, relying on each transformation on operations to be
+designed with the semantics in mind.  Preferably, the semantic
+properties are described abstractly using [Traits](Traits.md) and
----------------
mehdi_amini wrote:
> > each transformation on operations to be designed with the semantics in mind
> 
> I don't understand what this means here.
I've rewritten this section.


================
Comment at: mlir/docs/LangRef.md:211
+Value identifiers are only in scope for the region in which they are
+defined. Argument identifiers in mapping functions are in scope for
+the mapping body.  Particular operations may further limit the valid
----------------
mehdi_amini wrote:
> >  Argument identifiers in mapping functions
> 
> Not sure what "mapping function" refers to here? It is in the existing doc, but this is the only place it is used. I wonder if it isn't something remaining from before functions were regular Operations?
> 
> Is this about block argument? Can you just write "Block argument" instead? 
> 
> Edit: I wonder if this is about affine maps?
> (we can leave this out of this revision, but we should probably revisit this)
Yes, I believe this is talking about affine maps.


================
Comment at: mlir/docs/LangRef.md:213
+the mapping body.  Particular operations may further limit the valid
+scope of identifiers. For instance, the scope of values in a region
+with [SSA control flow semantics](#control-flow-and-ssacfg-regions) is
----------------
mehdi_amini wrote:
> > Particular operations may further limit the valid scope of identifiers
> 
> I think this is not specific enough: the scope is only limited by the region kind and I'd rather spell it out this way.
> With how you wrote it, one could think that we could have an operation restricting the scope of the value it produces (for example to be block-local).
> 
You're right, that could be misleading.  I've rewritten it.


================
Comment at: mlir/docs/LangRef.md:339
 An MLIR Module represents a top-level container operation. It contains
-a single region containing a single block which can contain any
-operations.  Operations within this region must not implicitly capture
-values defined outside the module.  Modules have an optional symbol
-name that can be used to refer to them in operations.
+a single [SSACFG region)[#control-flow-and-ssacfg-regions]
+containing a single block which can contain any operations. Operations
----------------
mehdi_amini wrote:
> It isn't clear to me why a module wouldn't be a graph region?
> Do we put any meaning to the order of operations in a module?
I suppose it could be, although it's somewhat of a degenerate kind of region anyway, since modules don't define any values and don't contain blocks.  Maybe this should be another kind of region?


================
Comment at: mlir/docs/LangRef.md:478
 [[more rationale](Rationale/Rationale.md#block-arguments-vs-phi-nodes)].
 
 ## Regions
----------------
mehdi_amini wrote:
> I feel the section here isn't entirely complete with respect to non CFG region with multiple blocks: are these still having terminator? What's the contract on block arguments?
> 
> An example would be worthwhile as well.
I added an additional note about the Graph Region rationale, but I'm really not sure how to provide an example here of something that I don't have a good example of :).  Let's chat offline about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358