[PATCH] D24226: [PM] Provide an initial, minimal port of the inliner to the new pass manager.

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

Thanks! Mostly minor comments below.



================
Comment at: include/llvm/Transforms/IPO/Inliner.h:1
-//===- InlinerPass.h - Code common to all inliners --------------*- C++ -*-===//
+//===- LegacyInlinerBase.h - Inliner pass and infrastructure ----*- C++ -*-===//
 //
----------------
The header above should read Inliner.h


================
Comment at: include/llvm/Transforms/Utils/Cloning.h:200
+  /// 'InlineFunction' fills this in by scanning the inlined instructions, and
+  /// only of CG is null. If CG is non-null, instead the value handle
+  /// `InlinedCalls` above is used.
----------------
typo: of -> if


================
Comment at: lib/Transforms/IPO/Inliner.cpp:857
+      // Add any new callsites to the worklist.
+      Calls.append(IFI.InlinedCallSites.begin(), IFI.InlinedCallSites.end());
+
----------------
sanjoy wrote:
> I'm not confident that the calls sites you've put in `Calls` here and above will stay valid across the future iterations of this loop.  That is, say we're looking at the `main` SCC in:
> 
> ```
> void f() { return false; }
> void g() { }
> void main() {
>   if (f() /* CS0 */)
>     g() /* CS1 */;
> }
> ```
> 
> Before entering the loop, we'll put `CS0` and `CS1` in `Calls`.  Once we inline through `CS0`, it is reasonable (though I don't know if it does this today) for `InlineFunction` to want to simplify away and delete `CS1`, leaving a dangling pointer in `Calls`.
> 
> Is there a reason why that ^ won't happen?
When Calls is initially populated, only callees for which isDeclaration is false is added. That is not the case with the callees in IFI.InlinedCallSites. We check and bail out if Callee->isDeclaration() is true in many places (getInlineCost for example), so this probably doesn't break anything now, but it's preferable to filter out callees without their body when we augment Calls above. 


================
Comment at: lib/Transforms/IPO/Inliner.cpp:881
+    // callee.
+    for (Function *InlinedCallee : InlinedCallees) {
+      LazyCallGraph::Node &CalleeN = *CG.lookup(*InlinedCallee);
----------------
This might add edges that don't exist anymore in the IR: assuming a->b->c originally, and b first gets inlined and then c gets inlined into 'a' this would still add a->c edge. 


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1645
       UpdateCallGraphAfterInlining(CS, FirstNewBlock, VMap, IFI);
+    } else {
+      // Otherwise just collect the raw call sites that were inlined.
----------------
Partial inlining implementation creates IFI with an empty CG and this code unnecessarily adds the inlined callsites when invoked in the context of partial inlining. Not a big deal, but may be you can change InlinedCallSites to a pointer and guard the below code based on that?


https://reviews.llvm.org/D24226