[PATCH] D155711: [SimplifyCFG] Hoist common instructions on Switch.

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

This looks pretty good to me. Only significant concern I have is the unreachable handling, which doesn't look quite correct to me.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1531
+  // instructions in the two blocks. In particular, we don't want to get into
+  // O(∏N) situations here where N is the sizes of these successors. As
   // such, we currently just scan for obviously identical instructions in an
----------------



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1549
 
-  bool Changed = false;
-
-  auto _ = make_scope_exit([&]() {
-    if (Changed)
-      ++NumHoistCommonCode;
-  });
+  // The second of pair record any skipped instuctions that may read memory,
+  // write memory or have side effects, or have implicit control flow.
----------------
instuctions -> instructions

Also this sounds like it's a count of instructions, while this is really a SkipFlags bitmask.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1584-1590
+      for (auto *Iter = SuccIters.begin(); Iter != SuccIters.end();) {
+        if (isa<UnreachableInst>(*Iter->first)) {
+          Iter = SuccIters.erase(Iter);
+        } else {
+          ++Iter;
+        }
+      }
----------------



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1590
+        }
+      }
+
----------------
Unless I'm missing something, I don't think this code is correct in the case where instructions are skipped. E.g. if the block is something like `call @not_willreturn; unreachable` and we skip over the first instruction and then see the unreachable, it would not be correct to ignore this successor. Is there a test for this?

I think you need to do the removal of unreachable blocks before the main loop, so you only handle the case where there is only `unreachable` in the block, not any other instructions.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1610
+      }
+    }
+
----------------
I think this may misbehave in the following case: You have BB1 and BB2 with same debug info and BB3 with different. Then BB1 and BB3 will skip it, but BB2 won't.

So I think you want to first do a check whether all are identical, and then skip or not skip debug info.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1631
+        Insts.insert(&*SuccIter.first);
+      }
+      return hoistSuccIdenticalTerminatorToSwitchOrIf(TI, I1, Insts) || Changed;
----------------
Omit braces for single line body.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1649
+        }
+      }
     }
----------------
Could use all_of here.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1684
       Changed = true;
-      ++NumHoistCommonInstrs;
+      if (NumHoistCommonInstrs == 0)
+        NumHoistCommonCode += SuccIters.size();
----------------
This `== 0` on a statistic doesn't really make sense to me... This is a global variable, not just for this run of the function.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1704
+    Instruction *TI, Instruction *I1,
+    SmallPtrSetImpl<Instruction *> &OtherSuccTIs) {
+
----------------
Why does this use a set rather than a vector?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1757
+      OtherSuccTI->replaceAllUsesWith(NT);
+    }
     NT->takeName(I1);
----------------
Omit braces for one line if.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1768
+  } else {
+    std::vector<DILocation *> Locs(OtherSuccTIs.size() + 1);
+    Locs.push_back(TI->getDebugLoc());
----------------
SmallVector


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1773
+    }
+    NT->setDebugLoc(DILocation::getMergedLocations(Locs));
+  }
----------------
Can always use the second code path? Should also work for branches.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:7153
+        // if (HoistCommon && hoistCommonCodeFromSuccessors(BI,
+        // !Options.HoistCommonInsts))
         return requestResimplify();
----------------
Leftover?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155711/new/

https://reviews.llvm.org/D155711