[PATCH] D72223: [MLIR] Introduce affine.execute_region op

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.

Once again, this disappeared from my attention list... Is it due to "unresolved" grand-parent diff where I'm not listed as a reviewer?



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:630-631
+      blocks. The op's region can have zero or more arguments, each of which can
+      only be a memref. The operands bind 1:1 to its region's arguments. The op
+      can't use any memrefs defined outside of it, but can use any other SSA
+      values that dominate it. Its region's blocks can have terminators the same
----------------
bondhugula wrote:
> ftynse wrote:
> > I understand the idea of the restriction, but it looks like it can be circumvented by:
> > ```
> > %0 = ... : memref<...>
> > %1 = cheat.wrap %0 : memref<...> -> !cheat.opaque
> > affine.execute_region {
> >   // This defines the memref inside the region, so seemingly complies with the semantics.
> >   %2 = cheat.unwrap %1 : !cheat.opaque -> memref<...>
> >   affine.load %2[...]
> > }
> > ```
> Independently of affine.execute_region, such a problem exists with memref non-dereferencing ops and it can / has to be tackled. For example, consider this variation of your snippet (no affine.execute_region).
> ```
> %0 = ... : memref<...>
> %1 = cheat.wrap %0 : memref<...> -> !cheat.opaque
> %2 = cheat.load %1[0, 0] : !cheat.opaque
> cheat.store %v, %1[0, 0] : !cheat.opaque
> call @foo(%1) : (!cheat.opaque) -> ()
> affine.load %0[0, 0] : memref<...>
> ```
> Note that all the current passes/utilities including affine store to load fwd'ing, invariant load hoisting/scalar rep, dependence analysis itself, the affine loop fusion would do the wrong thing here because there is an escape side channel like you show. So, the thing you are pointing to is interesting and has to be tackled, but if we step back and take another look, this is the same as the larger issue of dealing with escaping or aliasing and not specific to execute_region. A solution to deal with these is to actually detect/treat unknown memref non-dereferencing ops that define SSA values (like your `cheat.unwrap`) and bail out in their presence (depending on what we are doing). For eg. the dependence information isn't going to be accurate in their presence. The point of explicit captures is that you know which memrefs are going in, but it isn't free of the problem of side-channel escapes that manifest in straightline code themselves. But whenever we don't have such escapes (and we can always detect that being conservative), which I believe is the common scenario, having the captures does serve its intended purpose -- you still won't have to look inside and do something special with these ops in all passes/utilities.
In the general case, I suppose it can be even worse than that.

```
func @foo(%arg0: !cheat.opaque, %arg1: memref<..>) { // opaque and memref alias
  cheat.do_cheat %arg0 // this affects the memref
  affine.load %arg1[...]
}
```

which looks like we either need a powerful and abstract enough way to describe the aliasing between objects of different types, or to treat any side-effecting operation conservatively.

Anyway, I agree with the argument that the proposed approach is no worse than what we already have in Affine and I would like to make progress on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72223