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

[Tobias Gysi via Phabricator via llvm-commits]

gysit added a comment.

Very nice improvement, we are getting there!

I have some additional nit comments and there is one suspicious test that is probably worth having a second look at.



================
Comment at: mlir/include/mlir/Transforms/CFGToSCF.h:83
+  /// unconditional branch.
+  virtual void createCFGSwitchOperation(Location loc, OpBuilder &builder,
+                                        Value flag,
----------------
ultra nit: createCFGSwitchOp for consistency?


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:197
+/// Class used to orchestrate creation of so-called edge multiplexers.
+/// This class creates a new basic blocks and routes all inputs edges
+/// to this basic block before branching to their original target.
----------------



================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:593
+    } else {
+      // A loop without an exit edges is a statically known infinite loop.
+      // Since structured control flow ops are not terminator ops, the caller
----------------



================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:725
+              Value succOperand = value;
+              if (!loopBlockDominates(*iter)) {
+                succOperand = getUndefValue(value.getType());
----------------
nit: drop braces.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:827
+    // the structured loop into it.
+    auto *newLoopHeader = new Block;
+    newLoopHeader->insertBefore(loopHeader);
----------------
ultra nit: Would it make sense to name it newLoopParentBlock?


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:845
+    auto builder = OpBuilder::atBlockBegin(newLoopHeader);
+    FailureOr<Operation *> scfOp = interface.createStructuredDoWhileLoopOp(
+        builder, oldTerminator, newLoopHeader->getArguments(),
----------------
ultra nit: I would rename to loopOp or even structuredLoopOp to avoid the scf.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:965
+
+  // Find all relevant edges and check the shape of the control flow graph at
+  // this point.
----------------
ultra nit.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:1004
+  //                  +---+    |      +---+     |
+  //                  |ret+<---+---->++   ++<---+
+  //                  +---+          |     |
----------------
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!


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:1115
+    // since the edge to the branch region is always dominating.
+    Block *regionEntryBlock = &conditionalRegion.front();
+    for (auto &&[oldValue, newValue] :
----------------
nit: subRegionEntryBlock or subRegionEntry to not confuse it with the regionEntry?


================
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
----------------
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?


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

https://reviews.llvm.org/D156889