[PATCH] D156889: [mlir][cf] Add ControlFlow to SCF lifting pass

[Markus Böck via Phabricator via llvm-commits]

zero9178 added inline comments.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:1004
+  //                  +---+    |      +---+     |
+  //                  |ret+<---+---->++   ++<---+
+  //                  +---+          |     |
----------------
gysit wrote:
> ultra nit:  Any chance there is a way to illustrate a bit better that the return is part of region1? Otherwise the graphics are super nice!
I tried to move the `ret` block further up and moving the `Region 1` label to have about equal distance to both in the hopes of it looking clearer


================
Comment at: mlir/test/Conversion/ControlFlowToSCF/test.mlir:291
+// CHECK-NEXT:   %[[IF_RES:.*]]:3 = scf.if %[[CALL_PAIR]]#0
+// CHECK-NEXT:     scf.yield %[[POISON]], %[[C1]], %[[C0]]
+// CHECK-NEXT:   else
----------------
gysit wrote:
> I would have expected that we return %[[ARG1]] instead of %[[POISON]] here since this should be the result of the control flow loop if I understand correctly?
Good catch! This was actually a bug :) The `transformToReduceLoop` method incorrectly handled loop header block arguments, accidently causing uses of the loop header block arguments outside the loop to be replaced by its values at the end of the loop body. This should be fixed now.


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

https://reviews.llvm.org/D156889