[MLIR] Don't insert YieldOp for non-void loop.for by default.

[pifon2a]

Summary

The ForOp::build ensures that there is a block terminator which is great for
the default use case when there are no iter_args and loop.for returns no
results. In non-zero results case we always need to call replaceOpWithNewOp
which is not the nicest thing in the world. We can stop inserting YieldOp when
iter_args is non-empty. IfOp::build already behaves similarly.

[pifon2a]

Reformat.

[ftynse]

mlir/lib/Dialect/LoopOps/LoopOps.cpp

51

This looks like the constructed op will be invalid by construction if iterArgs is non-empty, due to the missing terminator. It may be fine as long as it is clear from the documentation that the caller is supposed to fix that. Alternatively, you can still create a terminator that returns iterArgs, which would give you a valid region, but will probably defy the purpose of this patch.

[bondhugula]

Commit summary please - even if it's a couple of lines. It's not clear from the title why the 'for' block won't be missing a terminator.

[pifon2a]

Improved commit description

[pifon2a]

Updated docs in .td.

[pifon2a]

In D77389#1959653, @bondhugula wrote:

Commit summary please - even if it's a couple of lines. It's not clear from the title why the 'for' block won't be missing a terminator.

@bondhugula Updated the summary. Thanks!

mlir/lib/Dialect/LoopOps/LoopOps.cpp

51

I updated the documentation in LoopOps.td. The option with removing 'ensureTermitor' even for the default case without results did not work quite well, since it breaks many users and adds boilerplate code to insert YieldOp, since mostly, ForOp is used without `iter_args' and I think it makes sense to leave this use case with implicit insertion of terminator.

[ftynse]

I've been told we have the same for IfOp, so this behavior is at least consistent within the dialect.

[pifon2a]

Formatting.

[pifon2a]

https://github.com/llvm/llvm-project/commit/51e3709c2b9055fe9e6fc83fc7f056bd55b2aaa6

mlir/include/mlir/Dialect/LoopOps/LoopOps.td

let description = [{
     region capturing the loop body. The induction variable is represented as an
     argument of this region. This SSA value always has type index, which is the
     size of the machine word. The step is a value of type index, required to be
     positive.
     The lower and upper bounds specify a half-open range: the range includes
     the lower bound but does not include the upper bound.
 
     The body region must contain exactly one block that terminates with
-    "loop.yield".  Calling ForOp::build will create such a region and insert
+    "loop.yield". Calling ForOp::build will create such a region and insert
     the terminator implicitly if none is defined, so will the parsing even in
     cases when it is absent from the custom format. For example:
 
     ```mlir
     loop.for %iv = %lb to %ub step %step {
       ... // body
     }
     ```
 
     `loop.for` can also operate on loop-carried variables and returns the final
     values after loop termination. The initial values of the variables are
     passed as additional SSA operands to the "loop.for" following the 3 loop
     control SSA values mentioned above (lower bound, upper bound and step). The
     operation region has equivalent arguments for each variable representing
     the value of the variable at the current iteration.
 
     The region must terminate with a "loop.yield" that passes all the current
     iteration variables to the next iteration, or to the "loop.for" result, if
-    at the last iteration.  "loop.for" results hold the final values after the
-    last iteration.
+    at the last iteration. Note, that when the loop-carried variables are
+    present, calling ForOp::build will not insert the terminator implicitly.
+    The caller must insert "loop.yield" in that case.
 
+    "loop.for" results hold the final values after the last iteration.
     For example, to sum-reduce a memref:
 
     ```mlir
     func @reduce(%buffer: memref<1024xf32>, %lb: index,
                  %ub: index, %step: index) -> (f32) {
       // Initial sum set to 0.
       %sum_0 = constant 0.0 : f32
       // iter_args binds initial values to the loop's region arguments.

mlir/lib/Conversion/LoopToStandard/LoopToStandard.cpp

for (auto loop_operands :
     auto iterRange = forOp.getRegionIterArgs();
     iterArgs.assign(iterRange.begin(), iterRange.end());
 
     if (first) {
       // Store the results of the outermost loop that will be used to replace
       // the results of the parallel loop when it is fully rewritten.
       loopResults.assign(forOp.result_begin(), forOp.result_end());
       first = false;
-    } else {
-      // A loop is constructed with an empty "yield" terminator by default.
-      // Replace it with another "yield" that forwards the results of the nested
-      // loop to the parent loop. We need to explicitly make sure the new
-      // terminator is the last operation in the block because further
-      // transforms rely on this.
+    } else if (!forOp.getResults().empty()) {
+      // A loop is constructed with an empty "yield" terminator if there are
+      // no results.
       rewriter.setInsertionPointToEnd(rewriter.getInsertionBlock());
-      rewriter.replaceOpWithNewOp<YieldOp>(
-          rewriter.getInsertionBlock()->getTerminator(), forOp.getResults());
+      rewriter.create<YieldOp>(loc, forOp.getResults());
     }
 
     rewriter.setInsertionPointToStart(forOp.getBody());
   }
 
   // Now copy over the contents of the body.
   SmallVector<Value, 4> yieldOperands;
   yieldOperands.reserve(parallelOp.getNumResults());
for (auto &op : parallelOp.getBody()->without_terminator()) {
     mapping.map(reduceBlock.getArgument(1),
                 mapping.lookupOrDefault(reduce.operand()));
     for (auto &nested : reduceBlock.without_terminator())
       rewriter.clone(nested, mapping);
     yieldOperands.push_back(
         mapping.lookup(reduceBlock.getTerminator()->getOperand(0)));
   }
 
+  if (!yieldOperands.empty()) {
     rewriter.setInsertionPointToEnd(rewriter.getInsertionBlock());
-  rewriter.replaceOpWithNewOp<YieldOp>(
-      rewriter.getInsertionBlock()->getTerminator(), yieldOperands);
+    rewriter.create<YieldOp>(loc, yieldOperands);
+  }
 
   rewriter.replaceOp(parallelOp, loopResults);
 
   return success();
 }
 
 void mlir::populateLoopToStdConversionPatterns(
     OwningRewritePatternList &patterns, MLIRContext *ctx) {

mlir/lib/Dialect/LoopOps/LoopOps.cpp

 
 void ForOp::build(Builder *builder, OperationState &result, Value lb, Value ub,
                   Value step, ValueRange iterArgs) {
   result.addOperands({lb, ub, step});
   result.addOperands(iterArgs);
   for (Value v : iterArgs)
     result.addTypes(v.getType());
   Region *bodyRegion = result.addRegion();
+  bodyRegion->push_back(new Block());
+  if (iterArgs.empty())
     ForOp::ensureTerminator(*bodyRegion, *builder, result.location);
   bodyRegion->front().addArgument(builder->getIndexType());
   for (Value v : iterArgs)
     bodyRegion->front().addArgument(v.getType());
 }
 
 static LogicalResult verify(ForOp op) {
   if (auto cst = dyn_cast_or_null<ConstantIndexOp>(op.step().getDefiningOp()))
     if (cst.getValue() <= 0)

 }
 
 //===----------------------------------------------------------------------===//
 // IfOp
 //===----------------------------------------------------------------------===//
 
 void IfOp::build(Builder *builder, OperationState &result, Value cond,
                  bool withElseRegion) {
-    build(builder, result, /*resultTypes=*/llvm::None, cond, withElseRegion);
 }
 
 void IfOp::build(Builder *builder, OperationState &result,
                  TypeRange resultTypes, Value cond, bool withElseRegion) {
   result.addOperands(cond);
   result.addTypes(resultTypes);
 
   Region *thenRegion = result.addRegion();