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

Fri Apr 24 14:05:48 PDT 2020

bondhugula added inline comments.

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

================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:632-635
+      values that dominate it. Its region's blocks can have terminators the same
+      way as current MLIR functions (FuncOp) can. Control from any return ops
+      from the top level of its region returns to right after the
+      affine.execute_region op. Its control flow thus conforms to the control
----------------
ftynse wrote:
> I would avoid referring FuncOp, it is not special in any sense, and it actually allows any terminator. Neither would I remind that blocks must have terminators, it's a core IR requirement.
> 
> "returns to right after the affine.execute_region op" sounds unnecessarily complex to me. "std.return" terminator placed inside blocks of the "affine.execute_region" returns the control flow to the "affine.execute_region". Since we already mentioned that "execution_region" executes the region exactly ones, it is naturally implied that the control flow will be further transferred to the control-flow successor of "execution_region...
Sure, dropped these lines. 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:108
+static bool isTopLevelValue(Value value, Operation *opWithRegion) {
+  assert(opWithRegion->getNumRegions() > 0 &&
+         "only to be called on ops with regions");
----------------
ftynse wrote:
> Nit: wouldn't it be easier to accept the region as argument instead of the operation that contains it?
Actually, it is - thanks! 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:121
+static Operation *getAffineScope(Operation *op) {
+  // TODO: make this compact by introducing a variadic pack on getParentOfType.
+  auto *curOp = op;
----------------
ftynse wrote:
> Would variadic template help you? `isKnownIsolatedFromAbove` isn't a class...
Right - it won't; the comment is probably stale (it was FuncOp and AffineExecuteRegionOp earlier). Anyway this code will be updated to make use of a new op trait 'PolyhedralScope' in another revision, which I'll submit as this one's parent. AffineExecuteRegion will be marked with this trait and other ops that want to define new scopes can have that trait. 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:128
+
+  assert(false && "op doesn't have a parent op");
+  return nullptr;
----------------
ftynse wrote:
> The assert message is misleading. The op may have parent ops, just none that satisfy the "affine scope" conditions.
> 
> Also, use `llvm_unreachable` instead of `assert(false)` and drop the return value
Thanks. 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:169
+    auto *parentOp = value.cast<BlockArgument>().getOwner()->getParentOp();
+    return isa<AffineForOp>(parentOp) || isa<AffineParallelOp>(parentOp);
+  }
----------------
ftynse wrote:
> Shouldn't this also check for "KnownIsolatedFromAbove" ?
This was checked as part of isValidSymbol above. 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:1068

+  auto *scope = getAffineScope(*this);
   for (auto idx : getSrcIndices()) {
----------------
ftynse wrote:
> Nit: we tend to use `auto` when it increases readability, `Operation *` would look just fine here
Sure. 

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2302
+  }
+  body->addArguments(memrefTypes);
+  region->push_back(body);
----------------
ftynse wrote:
> If you have ValueRange, this entire vector manipulation gets replaced by `memrefs.getTypes()`
Sure, this was really old code - that didn't get updated post ValueRange/TypeRange migrations!

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2303
+  body->addArguments(memrefTypes);
+  region->push_back(body);
+
----------------
ftynse wrote:
> You already pushed it in line 2295
Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72223