[PATCH] D80358: [MLIR] Add RegionKindInterface

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
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:
> stephenneuendorffer wrote:
> > 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.
> The code above is actually legal, since "%foo_value" is legal to use as an argument to bar.dominance_region
The fairly simple rule that I've always applied to dominance is that the parent operations act as dominance synch points. The parent operation of the definition has to dominate the ancestor of the user that is within that region.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80358