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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:485
+    BasicBlock *TrueDest = BI->getSuccessor(0);
+    BasicBlock *FalseDest = BI->getSuccessor(1);
+    SmallPtrSet<BasicBlock *, 4> TrueDestSucc, FalseDestSucc;
----------------
Bail if `TrueDest == FalseDest`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:497
+      if (!TrueDestSucc.empty())
+        CommonSucc = *TrueDestSucc.begin();
+    }
----------------
If the size of set is not 1, you are introducing non-deterministic optimizer behavior here. Please assert that the size is 1.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:550
+    for (auto Pair : HoistableBranches) {
+      if ((Pair.first->getSuccessor(0) == BB && BB != Pair.second) ||
+          (Pair.first->getSuccessor(1) == BB && BB != Pair.second)) {
----------------
`BB != Pair.second` is checked 2 times, you could instead write `if (BB != Pair.second && (succ1 == BB || succ2 == BB))`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:575
+    auto CreateHoistedBlock = [&](BasicBlock *Orig) {
+      if (HoistDestinationMap[Orig])
+        return HoistDestinationMap[Orig];
----------------
Do you ever add nullptr to this map? I guess it should be `.count(Orig)`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:592
+
+    // Link up these blocks with branches
+    if (!HoistCommonSucc->getTerminator()) {
----------------
Dot missing.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:626
+
+    // Now finally hoist BI
+    ReplaceInstWithInst(
----------------
Dot missing.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:670
+    unsigned int idx = 0;
+    for (unsigned int i = 0; i < Worklist.size(); ++i) {
+      auto IsPredecessorOfWorklistI = [&](DomTreeNode *P) {
----------------
It's `O(N^2)` in the worst case, which is not super-cool. If I understand this part correctly, you need to make sure that there is an element of `WorkList` that is a predecessor of another element of this WorkList. It can be done faster if you accumulate all elements of the WorkList into a set and do like

  for (el : Worklist)
    for (p : predecessors(el))
      if (set.contains(p)) we are done.

It will be `O(N)`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:681
+    }
+    BasicBlock *BB = Worklist[idx]->getBlock();
+    Worklist.erase(Worklist.begin() + idx);
----------------
How do you ensure that you have found something? Is it correct to do it if nothing was found and `idx` has a default value?


https://reviews.llvm.org/D52827