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

Fri Apr 24 06:59:41 PDT 2020

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: Kayjukh.

I am very sorry, for some reason this diff was not appearing on my phabricator todo list until recently.... Please do not hesitate to ping me by email if I take more than a couple of workdays to iterate.

In general, this change makes sense to me and looks like a proper extension of the affine modeling. My only design concern is that one can circumvent the explicit-capture mechanism for memrefs. A direct solution would be to disallow any values of memref type to be defined in the region, but I have not considered the implications of this on the expressiveness.

There are several places where some action is performed twice (e.g., parsing attribute dict), requesting change for these.

================
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
----------------
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[...]
}
```

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

================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:656
+      affine.for %i = 0 to %n {
+        affine.execute_region [] = () : () -> () {
+          // %pow can now be used as a loop bound.
----------------
Syntax nit: I'd consider omitting empty `[] = ()`, looks distracting

================
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");
----------------
Nit: wouldn't it be easier to accept the region as argument instead of the operation that contains it?

================
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;
----------------
Would variadic template help you? `isKnownIsolatedFromAbove` isn't a class...

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:128
+
+  assert(false && "op doesn't have a parent op");
+  return nullptr;
----------------
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

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:166-167
+  if (!op) {
+    // This value has to be a block argument for a FuncOp, affine.for,
+    // affine.parallel, or affine.polyfor.
+    auto *parentOp = value.cast<BlockArgument>().getOwner()->getParentOp();
----------------
I cannot relate this comment to the code below.

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

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

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

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2290
+void AffineExecuteRegionOp::build(Builder *builder, OperationState &result,
+                                  ArrayRef<Value> memrefs) {
+  // Create a region and an empty entry block. The arguments of the region are
----------------
Prefer ValueRange to ArrayRef<Value>

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2299
+  memrefTypes.reserve(memrefs.size());
+  for (auto v : memrefs) {
+    memrefTypes.push_back(v.getType());
----------------
Nit: drop trivial braces

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2302
+  }
+  body->addArguments(memrefTypes);
+  region->push_back(body);
----------------
If you have ValueRange, this entire vector manipulation gets replaced by `memrefs.getTypes()`

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

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2339
+
+  for (auto &argEn : llvm::enumerate(entryBlock.getArguments())) {
+    if (op.getOperand(argEn.index()).getType() != argEn.value().getType())
----------------
Avoid capturing `llvm::enumerate` by-reference. An enumerator only keeps iterators, so taking copying it is cheap, and we avoid running into potential problems with implicit lifetime extension

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2341
+    if (op.getOperand(argEn.index()).getType() != argEn.value().getType())
+      return op.emitOpError("type of one or more region arguments does not "
+                            "match corresponding operand");
----------------
Since you already have an enumerator, you might as well mention the position of the mismatching argument.

================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2404
+  // Parse the optional attribute list.
+  if (parser.parseOptionalAttrDict(result.attributes))
+    return failure();
----------------
You already parser the attr dict 4 lines above.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72223