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

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

zero9178 added a comment.

In D156889#4555089 <https://reviews.llvm.org/D156889#4555089>, @Mogball wrote:

> Have you tried importing random C programs through LLVMIR and see how this pass performs? It would be very interesting to see how it scales for large programs (I struggle to guess the complexity just by reading the code).

Purely implementation-wise, the pass is quadratic over the region nesting. There are two reasons:

- The algorithm is top-down, starting with outer-most loops and conditional regions, moving inwards. It therefore needs to visit and move `n - k` blocks for each subregion created (where n is the block count of the parent region).
- For each new region, the dominance tree is currently recalculated. This is a matter of implementation quality and can be improved by manually updating the dominance tree with the transformations performed in the future if necessary.

I have also run a quick, not very scientific, test of two LLVM IR input files, imported them into MLIR, lifted them to CF and run the pass over them. One I had lying around, the other is actually the `CFGToSCF.cpp` file compiled with clang (very meta :) ).
The smaller file had 1544 basic blocks, not counting entry blocks, measured with `grep -P "^\s*\^\w+" | wc -l`
The larger file had 3613 basic blocks, not counting entry blocks.
Operation count is useless metric as the algorithm is only sensitive to basic block count.

The smaller file took on average 10.71ms measured with 41 runs of `./mlir-opt --lift-cf-to-scf ~/CFGToSCF.mlir --mlir-timing > /dev/null 2> >(grep Lift >&2)`
The larger file took on average 14.47ms measured the same way.
So not a very large increase but at the same time, just these two runs is not really suffiecent to talk about the complexity of the algorithm in practice. 
I expect the region nesting to be comparatively very small in most programs.



================
Comment at: mlir/include/mlir/Transforms/CFGToSCF.h:37
+  /// left as is.
+  virtual Operation *createBranchRegionOp(OpBuilder &builder,
+                                          Operation *controlFlowCondOp,
----------------
gysit wrote:
> Mogball wrote:
> > gysit wrote:
> > > Would it make sense to prefix all methods that create SCF operation with SCF? E.g., for this one it may make sense to rename to `createSCFBranchOp` and below `createSCFLoopOp`. That way it may be a bit easier to follow in the code when new SCF stuff is created.
> > This is a generic interface and isn't specific to the SCF dialect (although the name of the interface is confusing SCF as in structured control flow with SCF the dialect...)
> I agree this is a bit confusing - in most places in this revision SCF actually refers to structured control flow an not to the dialect. Any suggestions for an alternative naming that does not collide with the dialect name is very welcome. Maybe StructuredCF?
I've gone ahead and at least put `Structured` in the name of these to make them distinct from the CFG ones


================
Comment at: mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:74
+
+    llvm::report_fatal_error(
+        "Cannot convert unknown control flow op to structured control flow");
----------------
Mogball wrote:
> 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).
Leaving unraised IR is somewhat problematic unless in the function op. I implemented it as signaling pass failure now. I see it as essentially a precondition violation of the interface.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:257
+    // be assigned the `getUndefValue`. The amount of block arguments and their
+    // offset is saved in the map for `redirectEdges` to transform the edges.
+    llvm::SmallMapVector<Block *, BlockEntry, 4> entries;
----------------
gysit wrote:
> nit: `redirectEdges` == `entries`? Would it make sense to rename to edgeToArgRangeMap, or similar?
I actually meant to refer to the `redirectEdge` method here.
`entries` I have renamed to `blockArgMapping` since that is essentially what it is: A mapping from the block arguments of an entry block to the block arguments in the multiplexer block.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:362
+    // create a dummy flag for the switch.
+    Value realDiscriminator = discriminator && caseArguments.size() > 1
+                                  ? discriminator
----------------
gysit wrote:
> isn't caseArgument.size() > 1 iff discriminator != 0? if yes I would probably simplify to:
> 
> ```
> if (caseArguments.size() == 1)
>   discriminator = getSwitchValue(0);
> ```
This is not guaranteed due to the `excluded` set. It can still cause the discriminator to be redundant if it leads to only having one successor. I nevertheless switched to an easier to read `if`.


================
Comment at: mlir/lib/Transforms/Utils/CFGToSCF.cpp:960
+  // are all single-entry regions.
+  // 3) Some branch regions have an edge to one or more continuation.
+  // This occurs if some branches end in the same return-like operation and some
----------------
gysit wrote:
> Reading the code it seems the condition is somewhat more complex:
> - Either only a subset has a continuation edge
> - Or not all continuation edges point to the same continuation block.
> is that correct?
> 
> I guess an example may be more intuitive to understand in this case.
I have added some ASCII art. Enjoy :-)


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

https://reviews.llvm.org/D156889