[PATCH] D106997: [IRSim][IROutliner] Detecting Similar Phi Nodes and Outlining

[Jessica Paquette via Phabricator via llvm-commits]

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