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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula requested changes to this revision.
bondhugula added a comment.
This revision now requires changes to proceed.

Mostly typos and comment fixes.



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:54
     The body region must contain exactly one block that terminates with
-    "loop.terminator".  Calling ForOp::build will create such region and insert
-    the terminator, so will the parsing even in cases when it is absent from the
-    custom format. For example:
+    "loop.yield".  Calling ForOp::build will create such region and insert
+    the terminator implicitly if none is defined, so will the parsing even in cases 
----------------
such a region


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:67
+    operands to the "loop.for" following the 3 loop control SSA values mentioned above
+    (lower bound, upper bound and step). The operation region will have equivalent arguments 
+    for each variable representing the value of the variable at the current iteration.
----------------
Nit: will have -> has


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:72
+    variables to the next iteration, or to the "loop.for" result, if at the last iteration.
+    "loop.for" result variables hold the final values after the last iteration. 
+    
----------------
Nit: result variables -> results


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:198
+    inserted implicitly. Otherwise, it must be explicit.
+    Also, if "loop.if" defines no values, the 'else' block is optional,
+    and may be omitted. 
----------------
Better if this is rephrased to: 

"Also, if "loop.if" defines one or more values, the 'else' block cannot be omitted."


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:259
     The body region must contain exactly one block that terminates with
-    "loop.terminator". Parsing ParallelOp will create such region and insert the
+    "loop.yield". Parsing ParallelOp will create such region and insert the
     terminator when it is absent from the custom format. For example:
----------------
such a region


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:359
+    is defined by the parent operation. 
+    If "loop.yield" has any operands, the parent op must define the same
+    number and types of the operands. 
----------------
Consider rephrasing to: 
"If ..., the operands must match the parent op's results."


================
Comment at: mlir/include/mlir/IR/OpImplementation.h:638
+  /// Parse a list of assignments of the form
+  /// (%x = %y : type, ...)
+  virtual ParseResult parseAssignmentList(SmallVectorImpl<OperandType> &lhs,
----------------
Terminate comments with a period. 


================
Comment at: mlir/include/mlir/IR/OpImplementation.h:638
+  /// Parse a list of assignments of the form
+  /// (%x = %y : type, ...)
+  virtual ParseResult parseAssignmentList(SmallVectorImpl<OperandType> &lhs,
----------------
bondhugula wrote:
> Terminate comments with a period. 
(%x1 = %y1 : type1, %x2 = %y2 : type2, ...)

to be clearer here. 


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:84
+
+  // If ForOp define values check that number and types of of defined
+  // values match ForOp initial iter operands and backedge basic block arguments
----------------
Nit: defines
comma after values. "that number" -> "that the number"
of of defined -> of the defined


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:153
 
+  // Parse the optional initial iteration arguments
+  SmallVector<OpAsmParser::OperandType, 4> regionArgs, operands;
----------------
Terminate comments with periods please. 


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

https://reviews.llvm.org/D74174