[PATCH] D106990: [IROutliner] Outlining branches with single entry and single exit

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/IROutliner.h:323
+    bool visitBranchInst(BranchInst &BI) { 
+      if (EnableBranches)
+        return true;
----------------
just return `EnableBranches`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:36
 
+extern cl::opt<bool> DoNotEnableBranches;
+
----------------
maybe "DisableBranches" instead?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:96
 
+  /// Keeping track of how many different branches outside the region the
+  /// regions in the group perform.
----------------
this sentence doesn't really make sense to me?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:159
+  if (!BackInst->isTerminator())
+    if (EndInst != BackInst->getNextNonDebugInstruction())
+      return;
----------------
fold this `if` into the `if` above?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:168
+
+  BasicBlock::iterator It = StartInst->getIterator();
+  while(PHINode *PN = dyn_cast<PHINode>(&*It)) {
----------------
Comments?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:213
 
   CandidateSplit = true;
 }
----------------
Move this above the `if` and `else`, then you can early return from the `if` portion?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:817
+  for (BasicBlock *Block : BE) {
+    for (BasicBlock *Succ : successors(Block)) {
+      if (!BBSet.contains(Succ)) {
----------------
this loop could be an `any_of`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:846
+        continue;
+      else {
+        Region.IgnoreRegion = true;
----------------
`else` isn't necessary here


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1445
+      Instruction *NewEndInst =
+          Region.Candidate->backInstruction()->getNextNonDebugInstruction();
+      IRInstructionData *NewEndIRID = new (InstDataAllocator.Allocate())
----------------
assert that this isn't `nullptr`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1498
 
-    // TODO: If in the future we can outline across BasicBlocks, we will need to
-    // check all BasicBlocks contained in the region.
-    if (IRSC.getStartBB()->hasAddressTaken())
+    bool BBHasAddressTaken = false;
+    for (IRInstructionData &ID : IRSC) {
----------------
comments for this bit?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1499
+    bool BBHasAddressTaken = false;
+    for (IRInstructionData &ID : IRSC) {
+      if (ID.Inst->getParent()->hasAddressTaken()) {
----------------
can this be an `any_of`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1526
+      if (!ID.Inst->isTerminator())
+        if (std::next(ID.getIterator())->Inst !=
+            ID.Inst->getNextNonDebugInstruction())
----------------
fold this `if` into the `if` above?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1613
+        continue;
+      if (FoundBlocks.contains(BB))
+        continue;
----------------
fold this `if` into the `if` above?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1774
+  BasicBlock *RewrittenBB = cast<Instruction>(InstAsUser)->getParent();
+  Region.PrevBB = RewrittenBB->getSinglePredecessor();
+  if (Region.PrevBB == InitialStart) {
----------------
assert that this isn't nullptr?


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

https://reviews.llvm.org/D106990