[PATCH] D37163: [LICM] sink through non-trivially replicable PHI

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'd like more eyes on this to figure out if there are potential problems with this transform (maybe there's a performance cost for splitting edges in some cases?).

A few more testcases would be nice; maybe a loop with multiple exit blocks, a loop with more than two exits pointing to the same exit block, and a loop where multiple exit PHI nodes use the same sinkable instruction.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:850
+  while (!PredBBs.empty()) {
+    BasicBlock *PredBB = *PredBBs.begin();
+    assert(CurLoop->contains(PredBB) &&
----------------
Iterating over a SmallPtrSet is non-deterministic.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:879
 
-#ifndef NDEBUG
-  SmallVector<BasicBlock *, 32> ExitBlocks;
-  CurLoop->getUniqueExitBlocks(ExitBlocks);
-  SmallPtrSet<BasicBlock *, 32> ExitBlockSet(ExitBlocks.begin(),
-                                             ExitBlocks.end());
-#endif
-
-  // Clones of this instruction. Don't create more than one per exit block!
-  SmallDenseMap<BasicBlock *, Instruction *, 32> SunkCopies;
-
-  // If this instruction is only used outside of the loop, then all users are
-  // PHI nodes in exit blocks due to LCSSA form. Just RAUW them with clones of
-  // the instruction.
-  while (!I.use_empty()) {
-    Value::user_iterator UI = I.user_begin();
+  for (Value::user_iterator UI = I.user_begin(), UE = I.user_end(); UI != UE;) {
     auto *User = cast<Instruction>(*UI);
----------------
Maybe put a comment here describing what the first loop is doing?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:925
+    Value::user_iterator UI = I.user_begin();
+    auto *User = cast<Instruction>(*UI);
+
----------------
Unnecessary cast.


https://reviews.llvm.org/D37163