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

[David Li via Phabricator via llvm-commits]

davidxl marked an inline comment as done.
davidxl added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:68
+  // The set of blocks in Entries that that are predecessors to NonReturnBlock
+  SmallVector<BasicBlock *, 4> NonReturnBlockPreds;
+};
----------------
wmi wrote:
> I find NonReturnBlockPreds has been set but not used. 
Removed


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:222-223
+
+  if (!CheckAndNormalizeCandidate(OutliningInfo.get()))
+    return std::unique_ptr<FunctionOutliningInfo>();
+
----------------
wmi wrote:
> should we use an assertion here instead of an early exit, so we can catch mistake instead of giving up silently.
No.  This is designed in way such that the candidate found may be illegal (to simplify the code), so a check here is necessary.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:340-343
   ToExtract.push_back(NewNonReturnBlock);
   for (BasicBlock &BB : *DuplicateFunction)
-    if (&BB != NewEntryBlock && &BB != NewReturnBlock &&
-        &BB != NewNonReturnBlock)
+    if (!ToBeInlined(&BB) && &BB != NewNonReturnBlock)
       ToExtract.push_back(&BB);
----------------
wmi wrote:
> ToBeInlined(NewNonReturnBlock) is also false, so can we just do:
>   for (BasicBlock &BB : *DuplicateFunction)
>     if (!ToBeInlined(&BB))
>       ToExtract.push_back(&BB);
NewNonReturnBlock needs to be the head of the blocks to be cloned. It is already pushed in, so the filter is needed.


https://reviews.llvm.org/D32249