[PATCH] D75837: Introduce std.execute_region op

Thu Mar 19 04:17:23 PDT 2020

bondhugula added a comment.

Thanks for the reviews! Updated.

In D75837#1930465 <https://reviews.llvm.org/D75837#1930465>, @silvas wrote:

> Okay, let's land this without allowing explicit captures, given that's the most restrictive semantics. We can loosen it later if there's a compelling need.

This will still need the patch on the ReturnOp to land. On a related note, I think we shouldn't move this op to the loop dialect unless the latter is renamed first.

================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:1025
+
+  let verifier = ?;
+
----------------
silvas wrote:
> Can you add a verifier that the region doesn't have args?
> 
> Also, I'm not super familiar with ODS, but does this specification autogenerate a verifier that there are no operands, or does it just not check anything about the op's operands? If the latter, please change it so that the verifier checks that there are no operands to this op. 
ODS doesn't support regions yet, and so you are right that we'll need to verify for zero region arguments. (Added that as well as > 0 blocks check.) But for operands, with no operands in the ODS description, the auto-generation will mark the op with the ZeroOperands trait, and the latter's verifier will check for it.

================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1388
+
+//
+// (ssa-id `=`)? `execute_region` `->` function-result-type `{`
----------------
rriddle wrote:
> Use /// For top-level comments.
Sorry, I didn't understand. These aren't doc comments, but for the implementation. This entire comment para along with the "Ex:" should be ///?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75837