[PATCH] D80358: [MLIR] Add RegionKindInterface

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer added inline comments.


================
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:
> stephenneuendorffer wrote:
> > 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?"
> I see the structure as trivial: only the semantics is complex (i.e. reachability, control-flow, etc.).
> 
> So I'm not sure what's the point of long description of what could be shown with a few lines of grammar specification (or even 10 lines of C++).
I think the structure is actually harder to grok than you might think for outsiders.  Also, I think the syntactic issues of how it is represented as a grammar doesn't necessarily reflect what you need to know to use it from c++.  (In fact, I've been considering if these need to be separated further, having a separate page for an abstract description and leave most of what is currently in LangRef.md as a description of the concrete grammar syntax.) 

I think one of the things that  is missing right now is that the semantics is quite flexible, but not without bounds.  Also the syntax is quite flexible, but also not without bounds.  I see this as a step towards being explicit about those bounds, at least on the semantic side.


================
Comment at: mlir/lib/IR/Dominance.cpp:37-56
+    auto kindInterface = dyn_cast<mlir::RegionKindInterface>(op);
+    for (unsigned i = 0; i < op->getNumRegions(); i++) {
+      auto &region = op->getRegion(i);
       // Don't compute dominance if the region is empty.
       if (region.empty())
         continue;
+
----------------
FYI: This is a significant departure from the previous patch, which attempted to put the 'smarts' in the Verifier.  It seems like in order to deal properly with Mehdi's testcase where a graph region encloses an SSACFG region,  without duplicating alot of the traversal, that all kinds of regions need a notion of dominance.  It seems to me that the right notion of dominance for graph regions is that all values are in-scope in all blocks. so: all blocks dominate all other blocks and all values within a block dominate all other values.  This also highlights the fact that CSE needs some different machinery to properly handle Graph Regions, since it currently traverses using DominatorTrees.


================
Comment at: mlir/lib/IR/Verifier.cpp:235
+    // Dominance is only meaningful inside reachable blocks.
+    if (hasDominance && domInfo->isReachableFromEntry(&block))
       for (auto &op : block)
----------------
stephenneuendorffer wrote:
> rriddle wrote:
> > I don't think this is correct. You are missing checks for how dominance interacts across different regions. Even if there is no intra-region dominance, there are still constraints how values may be used across regions. For example, the operand values have to be defined in this region or an ancestor region. This is also missing checks on how verification happens when the value is defined in a region with dominance and used in a region without. The dominance relationship between the ancestor operation in the region with dominance must still be checked, i.e.,
> > 
> > ```
> > // region with dominance
> > func @foo() {
> >   // region without dominance
> >   bar.non_dominance_region {
> >     // Verifier failure: This is a non-dominating use.
> >     bar.use %foo_value
> >     bar.return
> >   }
> >   %foo_value = constant true
> > ```
> Right...  I think some more test cases are in order here.
I've added some tests for this.  The more problematic case is the reverse:

  // region without dominance
  bar.non_dominance_region() {
    // region with dominance
    bar.dominance_region {
      // Should this be illegal?
      bar.use %foo_value
      bar.return
    }
    %foo_value = constant true

It's not clear to me that there is a useful interpretation for this.  At the moment I've made this illegal, but it's something that could conceivably be relaxed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358