[PATCH] D76603: [MLIR] Insert loop.yield to IfOp regions only if it's void.

Mon Mar 23 14:43:33 PDT 2020

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

@nmostafa thank you!

================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:224
+    OpBuilder<"Builder *builder, OperationState &result, Value cond, "
+              "bool withElseRegion, ArrayRef<Type> resultTypes = {}">
   ];
----------------
nmostafa wrote:
> Any reason for this refactoring ? I prefer to have results before uses to be consistent with tbl-gen'ed builders. Also, use TypeRange instead of ArrayRef. 
It became a refactoring after I rebased. I had a similar patch last week to add a builder for IfOp with results, but I  did not send it out on Friday. Today I sent it out and then after rebasing I noticed your patch. I do not really care about order of resultTypes args. I care only about loop.yield not being inserted for non-void ifops.

================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:233
+                       results().empty() ? body.end() : std::prev(body.end()));
     }
     OpBuilder getElseBodyBuilder() {
----------------
nmostafa wrote:
> Is the condition here reversed ? If we built a void IfOp, the YieldOp is auto-inserted, else it is missing, and we insert at end of block. 
> 
thank you for noticing!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76603