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

Fri Aug 4 01:00:44 PDT 2023

definelicht added a comment.

This is really exciting! Here is a grammar pass (use more punctuation please!).

================
Comment at: mlir/include/mlir/Transforms/CFGToSCF.h:91
+  /// This is required by the transformation as the lifting process might create
+  /// control-flow paths where a SSA-value is undefined.
+  virtual Value getUndefValue(Location loc, OpBuilder &builder, Type type) = 0;
----------------

================
Comment at: mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:49-50
+    if (auto switchOp = dyn_cast<cf::SwitchOp>(controlFlowCondOp)) {
+      // `getCFGSwitchValue` returns a i32 that we need to convert to index
+      // fist.
+      auto cast = builder.create<arith::IndexCastUIOp>(
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:28
+// blocks are created. In that case the transformation leaves behind a
+// conditional control flow graph operating dispatching to the given regions
+// terminating with different kinds of return-like operations each.
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:31-33
+// If the function only contains a single kind of return-like operations,
+// it is guaranteed to lift all control flow graph ops to structured control
+// flow and that no more control flow graph ops remain after the operation.
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:48
+// flow to create single entry and exit blocks is achieved via a multiplexer
+// construct best visualized as follows:
+//                         +-----+ +-----+   +-----+
----------------
As much as I like ASCII art, calling it the "best" form of visualization might be an overstatement 😉 

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:61
+//
+// transforms to:
+//                         +-----+ +-----+   +-----+
----------------
You're not continuing any sentence here 🙂 

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:77-78
+//
+// bbM in the above is the multiplexer block and any block previously branching
+// to an entry block of the region are redirected to it. This includes any
+// branches from within the region. Using a block argument, bbM then dispatches
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:85-89
+// The above form has the advantage that bbM now acts as the loop header
+// of the loop body and after the transformation on the latch, results in a
+// structured loop that can then be lifted to structured control flow. The
+// conditional branches created in bbM are later lifted to conditional
+// branches.
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:204-208
+/// This constructs creates a new basic blocks and redirects all edges given to
+/// be routed through that basic block before branching to their original
+/// target. The purpose of this transformation is to create single-entry
+/// single-exit regions by rerouting multiple entry edges or multiple exit edges
+/// through an edge multiplexer.
----------------
gysit wrote:
> 
One sentence about what it does, one sentence about why (I would potentially even reverse the order)

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:420-422
+  /// If more than one kind of `returnLikeOperation` has been seen, transforms
+  /// `returnLikeOperation` to a branch to the only block in region with an
+  /// instance of `returnLikeOperation`s kind.
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:507-508
+
+/// Creates a single entry block out of multiple entry edges using a edge
+/// multiplexer and returns it.
+static EdgeMultiplexer
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:529
+/// A structured loop is a loop satisfying all of the following:
+/// * Has at most one entry, exit and back edge.
+/// * The back edge originates from the same block as the exit edge.
----------------

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:635-638
+/// Note that the requirement of (0) is equal to LCSSA form in LLVM. However,
+/// due to this being a structured loop instead of a general loop, we do not
+/// require complicated dominance algorithms nor SSA updating making this
+/// implementation easier than creating a generic LCSSA transformation pass.
----------------
Alternatively "similar to LCSSA form", but definitely not equal

================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:821-826
+/// Makes sure the branch region has only a single exit. This is required by the
+/// recursive part of the algorithm as it expects the CFG to be single-entry
+/// and single-exit. This is done by simply creating an empty block if there
+/// is more than one block with an edge to the continuation block. All blocks
+/// with edges to the continuation are then redirected to the block. A region
+/// terminator is later placed into the block.
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156889