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

Fri Apr 3 09:08:05 PDT 2020

pifon2a marked 2 inline comments as done.
pifon2a added a comment.

In D77389#1959653 <https://reviews.llvm.org/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!

================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:51
+  bodyRegion->push_back(new Block());
+  if (iterArgs.empty())
+    ForOp::ensureTerminator(*bodyRegion, *builder, result.location);
----------------
ftynse wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77389