[PATCH] D74174: [MLIR] Allow Loop dialect IfOp and ForOp to define values

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:368
+  let builders = [
+    OpBuilder<"Builder *builder, OperationState &result">
+  ];
----------------
Can you just provide the body of the builder here instead of the .cpp?


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:371
 
-  // Fully specified by traits.
-  let verifier = ?;
+  let printer = ?;
 }
----------------
Why are you not specifying a printer? Also, can you just use the declarative form? That would remove the need for defining the parser/printer in c++.

https://mlir.llvm.org/docs/OpDefinitions#declarative-assembly-format

Should be something like:
```
let assemblyFormat = "operands attr-dict `:` type(operands)";
```


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:88
+  auto opNumResults = op.getNumResults();
+  if (opNumResults != 0) {
+    if (op.getNumIterOperands() != opNumResults)
----------------
Can you switch this to early exit instead?


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:160
+  if (succeeded(parser.parseOptionalKeyword("iter_args"))) {
+    parser.parseAssignmentList(regionArgs, operands, argTypes);
+    // Resolve input operands.
----------------
This needs to be checked for failure, missing tests?


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

https://reviews.llvm.org/D74174