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

[DianQK via Phabricator via llvm-commits]

DianQK added a comment.

You may need to run `git clang-format HEAD~1`.
The `jump-table-islands.ll` test case looks like it needs updating.

> First time reviewer, please let me know if anything is wrong.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6206
     return true;
+  errs()<<"Type Legal for TTI\n";
 
----------------
Unnecessary debugging logs?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6758
+                               SmallPtrSetImpl<BasicBlock *> &CasePredBBs) {
+  BasicBlock *SuccBB = CaseBB->getSingleSuccessor();
+
----------------
SuccBB looks like it could be a parameter?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6762
+  // SuccBB is the single successor of CaseBB. Do not handle infinite loop.
+  if (!SuccBB || !CaseBBs.contains(SuccBB) || SuccBB == CaseBB ||
+      CaseBB->phis().empty() || SuccBB->phis().empty() ||
----------------
Can these checks be used to create a small enough `CaseBBs` at `MergePhisInSwitch`?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6775
+
+  for (PHINode &SuccPhi : SuccBB->phis()) {
+    auto *CasePhi = dyn_cast<PHINode>(SuccPhi.getIncomingValueForBlock(CaseBB));
----------------
Performing this check first may reduce unnecessary `CasePredBBs` updates?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6781
+
+  return true;
+}
----------------
Does it make sense to use a `Changed` variable to update multiple Cases at once?


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

https://reviews.llvm.org/D155940