[PATCH] D11481: [LoopUnswitch] Improve loop unswitch pass to find trivial unswitch conditions more effectively

[Bjarke Hammersholt Roune]

broune added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:770
@@ +769,3 @@
+  // the successor as well. Because in theory these successors should be
+  // fused to loop header block. The reason to not constant folding
+  // conditions in LoopUnswitch pass is that it could potentially break
----------------
It's not necessarily possible to combine the header with its successor even if it is an unconditional branch, since there might be other predecessors of the successor. It might also be good to point out that unswitching generates branches with constant conditions, so this is expected and not just some corner case.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:777
@@ +776,3 @@
+  // trivial condition candidate (condition that is not a constant).
+  std::set<BasicBlock*> Visited;
+  Visited.insert(Header);
----------------
Set can make an allocation per insertion, which is slow. I'd consider SmallSet instead.

http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:803
@@ +802,3 @@
+    // instructions.
+    for (BasicBlock::iterator I :*Successor)
+      if (I->mayHaveSideEffects())
----------------
space missing after the colon.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:805
@@ +804,3 @@
+      if (I->mayHaveSideEffects())
+        return false;
+
----------------
This loop is duplicated from outside the loop. Would it make sense to move this loop to the top to to avoid the duplication? It would require a slightly different loop condition. The insert into Visited could also be de-duplicated.


http://reviews.llvm.org/D11481