[PATCH] D52827: [LICM] Make LICM able to hoist phis

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I have some minor comment, only the one that is related to `SafetyInfo` worries me (yet it just needs using utility function instead of just `moveBefore`). All other things are non-functional nits.
I'll take some time running fuzz tests with this patch because it's big. :) Please wait few hours, I will either give you approval (under condition that my comments will be addressed) or give you a failing test example.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:504
+        auto It = std::find_if(F->begin(), F->end(), IsSucc);
+        assert(It != F->end());
+        CommonSucc = &*It;
----------------
Minor: please add explanatory messages to the assertions you make.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:562
+        assert(!BI && "BB is expected to be the target of at most one branch");
+        BI = Pair.first;
+      }
----------------
If you only expect BI to be the only element with the property you need, it would make sense to save some compile time by inserting `break` here. But on the other hand, the assertion you make also makes sense. How about this: `BI = find_if(<cond>)`, and then assert that `find_if(<cond> && el != BI) == HoistableBranches.end()`? Just a suggestion.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:797
+                        << ": " << *I << "\n");
+      I->moveBefore(HoistPoint);
+      HoistPoint = I;
----------------
Removal/insertion of instructions to blocks may also need SafetyInfo updates. We have a helper function `eraseInstruction` for removal that handles it properly, but there was no helper for `moveBefore` because of only 1 occurrence. I've just added it in the patch rL346472

I am pretty sure that the current code is accidentally correct because rehoisting happens outside of loop (and `SafetyInfo` doesn't really care), but please rebase on top of this patch and use this utility whenever you move instructions to keep things general.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:805
+#ifndef NDEBUG
+  LI->verify(*DT);
+  assert(DT->verify(DominatorTree::VerificationLevel::Fast));
----------------
It makes more sense to verify `DT` before `LI` since `LI` uses it.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1369
 static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
-                  ICFLoopSafetyInfo *SafetyInfo, OptimizationRemarkEmitter *ORE) {
-  auto *Preheader = CurLoop->getLoopPreheader();
+                  BasicBlock *Preheader, ICFLoopSafetyInfo *SafetyInfo,
+                  OptimizationRemarkEmitter *ORE) {
----------------
After you've changed the semantics, `Preheader` is no longer the preheader of `CurLoop` (or is it)? If not, please rename it. If yes, why do you need it as parameter? You can just take loop's preheader as it was before.

In any case, it should be `const BasicBlock *`.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827