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

[Jeff Niu via Phabricator via llvm-commits]

Mogball added a comment.

Tour de force! This is super cool. A welcome addition to the upstream library of passes.



================
Comment at: mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:74
+
+    llvm::report_fatal_error(
+        "Cannot convert unknown control flow op to structured control flow");
----------------
It seems like a better API would be to make all these functions `FailureOr<Operation *>` and then propagate the failure up the CFGToSCF pass itself. Not all structured CF dialects will be able to support everything the pass requires. 

Either having the pass fail entirely or leave unraised CFGs in the IR seems better to me (I don't know how hard the latter option would be).


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:142
+static void addBlockArgumentsFromOther(Block *block, Block *other) {
+  block->addArguments(
+      other->getArgumentTypes(),
----------------
I'm personally not a fan of the `addArguments` API. It's cheaper to just loop over the arguments of `other` and call `addArgument` one-at-a-time because it saves the constructor of the vector


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:185
+/// Returns a range of all edges from `block` to each of its successors.
+auto successorEdges(Block *block) {
+  return llvm::map_range(llvm::seq(block->getNumSuccessors()),
----------------
This should be a `static` method at the top-level


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:212
+  function_ref<Value(unsigned)> getSwitchValue;
+  function_ref<Value(Type)> getUndefValue;
+
----------------
It would be great to get some documentation on the fields


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:245
+  create(Location loc, ArrayRef<Block *> entryBlocks,
+         llvm::function_ref<Value(unsigned)> getSwitchValue,
+         llvm::function_ref<Value(Type)> getUndefValue,
----------------
`function_ref` is mapped into the MLIR namespace


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:463
+    // IR transformations have occurred if any exit blocks were created.
+    return llvm::any_of(llvm::make_second_range(mapping),
+                        llvm::identity<Block *>{});
----------------
Would it be cheaper to just keep a flag/counter around?


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:474
+  CyclesEdges result;
+  SmallPtrSet<Block *, 8> entryBlock;
+
----------------



================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:478
+  // or predecessors are from outside the cycles.
+  for (Block *block : cycles) {
+    for (auto pred = block->pred_begin(); pred != block->pred_end(); pred++) {
----------------
I'm not sure how big the SCCs get in practice, but I think DFS over the SCC would have better complexity  (O(E+V) I think) than looping over every in/out edge of every block 


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:531
+/// * The back edge originates from the same block as the exit edge.
+struct StructuredLoopProperties {
+  /// Block containing both the single exit edge and the single back edge.
----------------



================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:649
+
+  std::array<MutableOperandRange, 2> latchSuccessorOperands = [&] {
+    unsigned loopHeaderIndex = 0;
----------------
The lambda here seems unnecessary given that it only has one return


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:716
+  for (auto iter = loopHeader->pred_begin(); iter != loopHeader->pred_end();
+       iter++) {
+    // Latch successor arguments have already been handled.
----------------



================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:1075
+
+  for (Operation &operation : llvm::make_early_inc_range(region.getOps())) {
+    if (!operation.hasTrait<OpTrait::ReturnLike>())
----------------
A minor optimization here would be to only check the terminator operations of each block in the region.


================
Comment at: mlir/test/Conversion/ControlFlowToSCF/test.mlir:530
+// CHECK:      %[[WHILE:.*]]:{{.*}} = scf.while
+// CHECK-SAME: %[[ARG1:.*]] = %[[IF]]#0
+// CHECK-SAME: %[[ARG2:.*]] = %[[IF]]#1
----------------
Yeah I was afraid of something like this when I skimmed through the algorithm. "Perfect reconstructibility" comes at a cost, but I still think this pass will be very useful when the input is "mostly nice".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156889