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

[Chen Li]

chenli 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
----------------
broune wrote:
> 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.
Well, I think you can always combine them with some kind of successor duplications if it has multiple predecessors. But it will add too much complexity and it's not worth taking that approach :)

I will add you second comment in code.

================
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);
----------------
broune wrote:
> 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
Will do.

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

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:805
@@ +804,3 @@
+      if (I->mayHaveSideEffects())
+        return false;
+
----------------
broune wrote:
> 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.
Will do.


http://reviews.llvm.org/D11481