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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks fine to me, but I think this is currently missing tests for the "hoist not first instruction" case for switch? We should have at least basic tests that this is supported and side effects are checked.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1571
 
+  bool Changed = false;
+
----------------
I'd move this variable below the following if check (to before `for (;;)`).


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1578
+  // If we find an unreachable instruction at the beginning of a basic block, we
+  // can still hoist the instruction at the rest of the basic blocks.
+  if (SuccIters.size() > 2) {
----------------
"hoist the instruction at the rest" -> "hoist instructions from the rest"?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1595
+    // Skip debug info if it is not identical.
+    bool AllDbgInstsAreIdentical = llvm::all_of(
+        iterator_range(SuccIterBegin, SuccIters.end()), [I1](const auto &Pair) {
----------------
Drop llvm:: prefix.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1596
+    bool AllDbgInstsAreIdentical = llvm::all_of(
+        iterator_range(SuccIterBegin, SuccIters.end()), [I1](const auto &Pair) {
+          Instruction *I2 = &*Pair.first;
----------------
You are using `iterator_range(SuccIterBegin, SuccIters.end())` in a lot of places. I think it may make sense to create a variable for this?

There is also `make_first_range()` to iterate over `.first`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155711