[PATCH] D34200: [PM/unswitch] Teach SimpleLoopUnswitch to do non-trivial unswitching, making it no longer even remotely simple.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Drive-by review of part of the first file. This is... a lot of code.



================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:91
+/// FIXME: Should consider hand-rolling a slightly more efficient non-DFS
+/// approach here as we can dod that easily by persisting the candidate IDom's
+/// dominating set between each predecessor.
----------------
s/dod/do/ ?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:771-773
+  // Recurse down through further child loops.
+  for (Loop *ChildL : OrigL)
+    cloneLoop(*ChildL, ClonedL, VMap, LI);
----------------
What is the typical depth for these things? i.e. is there a risk that a sufficiently complex loop within a function will cause this to fail spectacularly? Will it be more complex to actually do a DFS using a stack and iteratively going through the blocks?


================
Comment at: lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:1004
+
+void recursivelyEraseNode(DominatorTree &DT, BasicBlock *BB) {
+  SmallVector<DomTreeNode *, 4> Worklist;
----------------
The name seems to be inconsistent with the behaviour -- i.e. it's not really recursively calling itself. Maybe remove the 'recursively' in the name? Also, why isn't this function static unlike the others?


https://reviews.llvm.org/D34200