[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 llvm-commits at lists.llvm.org
Wed Jun 14 11:24:45 PDT 2017

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?


More information about the llvm-commits mailing list