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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Okay, I think the logic here is mostly correct, though I'm not sure whether the overall framing for the transform is the right one.

I think the core of the transform is the following:

1. We have a block A containing only phi nodes.
2. With an unconditional branch to a block B that uses the phi nodes from A only in phi nodes.
3. For edges PA -> A, PB -> B we have two cases:

3a. PA != PB. This is the straightforward case, where we can just redirect PA -> B without causing a phi conflict.
3b. PA == PB. This is the interesting case. We keep both edges here, and keep the phi node in A. If there is just one such conflict, then the phi node in A because a single-element phi, and we can remove it entirely. Otherwise, we should have to keep the phi. In that case the transform is probably(?) not worthwhile, so we should limit to the case where A and B have a single common predecessor.

This makes the root of the transform the "uncond branch with only phis" block, and has no direct relation to switch instructions. This would be pretty close to TryToSimplifyUncondBranchFromEmptyBlock(), just for the case where we have a conflict on one predecessor, and as such cannot actually remove the block completely, but can rewrite in order to remove the phi nodes.

Does this framing make sense to you?



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6752
+static bool canMergePhisOfCase(BasicBlock *CaseBB,
+                               const SmallPtrSet<BasicBlock *, 16> &CaseBBs,
+                               SwitchInst *CommonSource,
----------------
SmallPtrSetImpl for arguments


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6767-6773
+    if ((!PredBB->getSingleSuccessor() || !CaseBBs.contains(PredBB)) &&
+        PredBB != CommonSource->getParent()) {
+      return false;
+    }
+
+    if (PredBB != CommonSource->getParent())
+      CasePredBBs.insert(PredBB);
----------------



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6780
+      return false;
+    }
+  }
----------------
Omit braces


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6784
+  return true;
+}
+
----------------
Unless I'm missing something, nothing here prevents CaseBB from dominating SuccBB, in which case there might be uses of phis from CaseBB outside of phis in SuccBB. Is the transform valid in that case?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6837
+      std::vector<DominatorTree::UpdateType> Updates;
+      Updates.reserve(PredBBs.size());
+      for (auto *PredBB : PredBBs) {
----------------
2 *


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155940