[PATCH] D106993: [IROutliner] Outline across branches with multiple exits

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:163
+
+      return LHSVal < RHSVal;
+    });
----------------
probably don't need variables here?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:462
+  Type *RetTy = Type::getVoidTy(M.getContext());
+  // All extracted functions _should_ have the same return type at this point.
+  // since the similarity identifier ensures that all branches outside of the
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:474
+      RetTy = ExtractedFuncType;
+    else if (RetTy->isIntegerTy(1) && RetTy->isIntegerTy(16))
+      RetTy = ExtractedFuncType;
----------------
the `if` + `else if` here set the same value; merge them into one `if`?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:543
        CurrBB = NextBB) {
+    DebugInsts.clear();
     NextBB = std::next(CurrBB);
----------------
comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:548
     Instruction *I = CurrBB->getTerminator();
-    if (isa<ReturnInst>(I))
-      NewEnd = &(*CurrBB);
+    if (ReturnInst *RI = dyn_cast<ReturnInst>(I)) {
+      Value *RetVal = RI->getReturnValue();
----------------
comment?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:550
+      Value *RetVal = RI->getReturnValue();
+      NewEnds.insert(std::make_pair(RetVal, &(*CurrBB)));
+    }
----------------
Might as well drop the variable here, since it's only used once.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1247
   unsigned MatchingNum = 0;
-  for (BasicBlock *CompBB : OutputStoreBBs) {
+  // We compare the new set output blocks to the other sets of outut blocks.
+  // If they are the same number, and have identical instructions, they are
----------------



================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1252
     WrongInst = false;
-    if (CompBB->size() - 1 != OutputBB->size()) {
-      WrongSize = true;
-      MatchingNum++;
-      continue;
-    }
-
     WrongSize = false;
+    for (std::pair<Value *, BasicBlock *> &VToB : CompBBs) {
----------------
Do you actually need two variables here? Why not just one called "Mismatch" or something?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1364
+  
+    if (NewBB->size() == 0) {
+      NewBB->eraseFromParent();
----------------
It looks like there's some similar code to this later down the line. Can you refactor this a bit?


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:1608
+      BasicBlock *NewBB = BasicBlock::Create(
+          M.getContext(),
+          Twine("output_block_") + Twine(static_cast<unsigned>(Idx)) +
----------------
This pattern (creating a BB with a name + indices then inserting into a list) appears a lot. Would it make sense to have a helper function for this?


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

https://reviews.llvm.org/D106993