[PATCH] D80358: [MLIR] Add RegionKindInterface

Wed Jun 17 20:31:09 PDT 2020

mehdi_amini added inline comments.

================
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:
> > 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.
> Yes, exactly

I was pointing out something that seems contradictory, so I'm puzzled by your answer here?

> Even in a graph, the IR still maintains an order, which enables deterministic round-tripping, for instance.

Yes but there is still no total order since you have cycles.

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

OK but the way it is written is still confusing to me. You're also using "Region" as if it is a well defined term while it is the first use in the doc.
I'll look for an alternative writing after we settle on the description of multi-blocks region below.

Operations are also organized into [Blocks](#blocks) and are totally ordered
within their containing [Region](#regions)

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

This complicates the design, and I don't believe your specification is complete right now with respect to non-CFG multi-blocks region.

The paragraph that follows is a good example, when it says that `when control flow enters a region, it always begins in the first block of the region, called the *entry* block` we already have a notion of control for the blocks. Then `Control flow can only pass to one of the specified successor blocks as in a branch operation, or back to the containing operation as in a return operation. Terminator operations without successors can only pass`.
In your example above:
```
"unregistered_op" () ({
^bb1:
  %2 = %1
  %1 = %2
^bb2:
  %3 = %2
}
```

The second block is clearly unreachable and can just be deleted.

It also means that:

```
"unregistered_op" () ({
^bb1:
  %2 = %3
  %1 = %2
^bb2:
  %3 = ...
}
```

Would be invalid IR.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358