[PATCH] D75837: [MLIR] Introduce std.execute_region op

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.

I added some more nits, mostly to keep this consistent with the changes coming in D76743 <https://reviews.llvm.org/D76743>

Also, std.yield seems good.



================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:933
+  let description = [{
+    The execute_region operation executes the region held exactly once. The op
+    cannot have any operands, nor does its region have any arguments. All SSA
----------------
nit: wrap this in ``.


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:944
+
+    Ex:
+
----------------
nit: Ex: -> Example


================
Comment at: mlir/include/mlir/Dialect/StandardOps/IR/Ops.td:947
+    ```mlir
+      loop.for %i = 0 to 128 {
+        %y = execute_region -> i32 {
----------------
nit: Don't indent inside of the mlir code block.


================
Comment at: mlir/lib/Dialect/StandardOps/IR/Ops.cpp:1388
+
+//
+// (ssa-id `=`)? `execute_region` `->` function-result-type `{`
----------------
bondhugula wrote:
> 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 ///?
Yeah for consistency, we use /// everywhere.


================
Comment at: mlir/test/IR/core-ops.mlir:605
+  // CHECK-NEXT:    br ^bb1
+  // CHECK-NEXT:  ^bb1:       // pred: ^bb0
+  // CHECK-NEXT:    return
----------------
nit: Don't check the pred comment,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75837