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

[Hongyu Chen via Phabricator via llvm-commits]

XChy added inline comments.


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


================
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() ||
----------------
DianQK wrote:
> Can these checks be used to create a small enough `CaseBBs` at `MergePhisInSwitch`?
I don't think it's necessary to create a small enough CaseBBs here. Instead, CaseBBs should include the successors of all cases, in order to guarantee correct checks like `All predecessors of CaseBB is also the successor of one case. SuccBB is  the successor of one case.`


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


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6781
+
+  return true;
+}
----------------
DianQK wrote:
> Does it make sense to use a `Changed` variable to update multiple Cases at once?
It may be much more complex to update them at once. There may be some diamond-like CFG or loops in Cases. Updating at once would bring some conflicts here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155940