[PATCH] D32249: [PartialInl] Enhance partial inliner to handle more complex conditions

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:43
+    "max-num-inline-blocks", cl::init(5), cl::Hidden,
+    cl::desc("Max Number of Blocks  To be Partialled Inined"));
+
----------------
typo, `partially inlined`. BTW, do you have numbers for different values of `MaxNumInlineBlocks` ?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:66-68
+  std::vector<BasicBlock *> ReturnBlockPreds;
+  // The set of blocks in Entries that that are predecessors to NonReturnBlock
+  std::vector<BasicBlock *> NonReturnBlockPreds;
----------------
`SmallVector<>` maybe



================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:120-131
+  auto IsSuccessor = [](BasicBlock *Succ, BasicBlock *BB) {
+    return is_contained(successors(BB), Succ);
+  };
+
+  auto SuccSize = [](BasicBlock *BB) {
+    return std::distance(succ_begin(BB), succ_end(BB));
+  };
----------------
These could probably live in `BasicBlockUtils`.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:134-139
+    if (IsReturnBlock(Succ1))
+      return std::make_tuple(Succ1, Succ2);
+    else if (IsReturnBlock(Succ2))
+      return std::make_tuple(Succ2, Succ1);
+    else
+      return std::make_tuple<BasicBlock *, BasicBlock *>(nullptr, nullptr);
----------------
Also this one. Please remove `else` after `return` 


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:162-166
+    if (SuccSize(CurrEntry) != 2)
+      break;
+
+    if (OutliningInfo->Entries.size() + 1 >= MaxNumInlineBlocks)
+      break;
----------------
Can you add comments about why we break in this case? (i.e. we don't outline blocks without exactly two successors, etc...)


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:203-205
+    for (auto E : OutliningInfo->Entries) {
+      Entries.insert(E);
+    }
----------------
braces unneeded.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:319
+      RetPhi->addIncoming(&*I, PreReturn);
+      for (auto E : OutliningInfo->ReturnBlockPreds) {
+        BasicBlock *NewE = cast<BasicBlock>(VMap[E]);
----------------
explicit type maybe?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:326
+    }
+    for (auto E : OutliningInfo->ReturnBlockPreds) {
+      BasicBlock *NewE = cast<BasicBlock>(VMap[E]);
----------------
explicit type maybe?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:335-337
+    if (BB == NewReturnBlock || NewEntries.count(BB))
+      return true;
+    return false;
----------------
`return BB == NewReturnBlock || NewEntries.count(BB)` , maybe?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:374-376
+    if (IsLimitReached()) {
       continue;
+    }
----------------
unrelated change, also braces are unneeded.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:401
 bool PartialInlinerImpl::run(Module &M) {
+
   if (DisablePartialInlining)
----------------
unrelated?


https://reviews.llvm.org/D32249