[PATCH] D106997: [IRSim][IROutliner] Detecting Similar Phi Nodes and Outlining
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 20 11:56:13 PST 2021
paquette added inline comments.
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:61
+
+ if (PHINode *PN = dyn_cast<PHINode>(Inst))
+ for (BasicBlock *BB : PN->blocks())
----------------
comment?
================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:106
+
+ for (unsigned Idx = 0; Idx < PN->getNumIncomingValues(); Idx++) {
+ BasicBlock *Incoming = PN->getIncomingBlock(Idx);
----------------
comment on the loop?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:189
+ DenseSet<BasicBlock *> &Included) {
+ for (PHINode &PN : PHIBlock->phis()) {
+ for (unsigned Idx = 0, PNEnd = PN.getNumIncomingValues(); Idx != PNEnd;
----------------
add in-code comments to the loop?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:197
+ Instruction *Term = Incoming->getTerminator();
+ BranchInst *BI = dyn_cast<BranchInst>(Term);
+ assert(BI && "Not a branch instruction?");
----------------
since `Term` is only used for computing `BI`, it might as well be folded in here.
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:199
+ assert(BI && "Not a branch instruction?");
+ for (unsigned Succ = 0, End = BI->getNumSuccessors(); Succ != End;
+ Succ++) {
----------------
needs comments?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:260
+ // If the region starts with a PHINode, but is not the original beginning of
+ // the branch instruction, we ignore this region for now.
+ if (isa<PHINode>(StartInst) && StartInst != &*StartBB->begin())
----------------
"original beginning go the branch instruction" doesn't make a lot of sense?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:268
+ EndBB = BackInst->getParent();
+ Instruction *LastPHI = &*std::prev(EndBB->getFirstNonPHI()->getIterator());
+ if (BackInst != LastPHI)
----------------
Is there a more succinct way to compute this?
================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:336
// branch at the end of PrevBB when we split the BasicBlock.
- PrevBB = StartBB->getSinglePredecessor();
+ /*PrevBB = StartBB->getSinglePredecessor();
assert(PrevBB != nullptr &&
----------------
Remove this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106997/new/
https://reviews.llvm.org/D106997
More information about the llvm-commits
mailing list