[PATCH] D155940: [SimplifyCFG] Transform for merging the combination of phis in switch

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Just some nits for now, I need to go over the logic carefully another time.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:284
   bool TurnSwitchRangeIntoICmp(SwitchInst *SI, IRBuilder<> &Builder);
+  bool MergePhisInSwitch(SwitchInst* SI, IRBuilder<>& Builder);
+
----------------
nit: wrong `&` formatting


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6750
+/// Try to transform a switch that has phi of phi in branches
+/// to the one with only one phi.
+///
----------------
drop "the"


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6771
+/// This converts a complex switch into a regular switch that
+/// has a common destination
+
----------------
Missing period at end of sentence.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6776
+
+  for (const auto &Case : SI->cases()) {
+    CaseBBs.insert(Case.getCaseSuccessor());
----------------
Omit braces


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6778
+    CaseBBs.insert(Case.getCaseSuccessor());
+  }
+
----------------
Using `successors()` instead of `cases()` would be more direct here?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6791
+
+    bool IsMatched = true;
+    SmallPtrSet<BasicBlock *, 16> PredBBs;
----------------
Move this logic into a separate function, so you can use early return instead of IsMatched.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6825
+      for (auto *PredBB : PredBBs) {
+        cast<BranchInst>(PredBB->getTerminator())->setSuccessor(0, SuccBB);
+        SuccPhi.addIncoming(CasePhi->getIncomingValueForBlock(PredBB), PredBB);
----------------
Just because something has a single successor, does not mean it's a plain branch.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6832
+      // Therefore, we need to erase CasePhi when it's not used
+      if (!CasePhi->hasNUsesOrMore(1))
+        CasePhi->eraseFromParent();
----------------
`CasePhi->use_empty()`?


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

https://reviews.llvm.org/D155940