[PATCH] D155940: [SimplifyCFG] Transform for merging the combination of phis in switch
DianQK via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 25 06:53:35 PDT 2023
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
More information about the llvm-commits
mailing list