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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks so much for the review! Updated patch and some responses below.



================
Comment at: lib/Transforms/IPO/Inliner.cpp:857
+      // Add any new callsites to the worklist.
+      Calls.append(IFI.InlinedCallSites.begin(), IFI.InlinedCallSites.end());
+
----------------
eraman wrote:
> sanjoy wrote:
> > eraman wrote:
> > > 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. 
> > If this is a reply to what I said above, then I'm not sure how `isDeclaration` is relevant to the problem I'm trying to point out.
> > 
> > I'm trying to sketch a scenario where `InlineFunction` ends up excising a call site to a function (that **has** a body) after we've put a pointer to the `CallInst` or `InvokeInst` for the said call site in `Calls` therefore ending up with a dangling pointer.
> > 
> > It is possible that `InlineFunction` does not do such simplifications (simplifications in blocks that logically belonged to the caller, that is); but if that is the case then that invariant should be documented and tested.
> Sorry, that wasn't a reply to your comment.  Mine was a separate unrelated comment on line 857 above.  
Nope, no reason other than "it doesn't do that that at the moment". At least, that I'm aware of...

I may be missing something, but a casual look through the old inliner's code makes me think it has the same fundamental assumption. Perhaps as a consequence, the current *inliner* does DCE on call instructions rather than InlineFunction doing this....

I think we should just document this (in a separate patch probably)...


================
Comment at: lib/Transforms/IPO/Inliner.cpp:857
+      // Add any new callsites to the worklist.
+      Calls.append(IFI.InlinedCallSites.begin(), IFI.InlinedCallSites.end());
+
----------------
chandlerc wrote:
> eraman wrote:
> > sanjoy wrote:
> > > eraman wrote:
> > > > 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. 
> > > If this is a reply to what I said above, then I'm not sure how `isDeclaration` is relevant to the problem I'm trying to point out.
> > > 
> > > I'm trying to sketch a scenario where `InlineFunction` ends up excising a call site to a function (that **has** a body) after we've put a pointer to the `CallInst` or `InvokeInst` for the said call site in `Calls` therefore ending up with a dangling pointer.
> > > 
> > > It is possible that `InlineFunction` does not do such simplifications (simplifications in blocks that logically belonged to the caller, that is); but if that is the case then that invariant should be documented and tested.
> > Sorry, that wasn't a reply to your comment.  Mine was a separate unrelated comment on line 857 above.  
> Nope, no reason other than "it doesn't do that that at the moment". At least, that I'm aware of...
> 
> I may be missing something, but a casual look through the old inliner's code makes me think it has the same fundamental assumption. Perhaps as a consequence, the current *inliner* does DCE on call instructions rather than InlineFunction doing this....
> 
> I think we should just document this (in a separate patch probably)...
Sanjoy, thanks for the thought. Sadly we rely on this already and just don't have any testing for it. I've added a test case based on your idea with lots of dead code to try and make sure we don't get this wrong at some point in the future. It also showcases exactly why we *should* do some DCE in the caller when inlining because we leave trivially dead code around.

I've also added a comment while I'm here so we don't forget. It's a small change.


Good thought Easwaran, totally agree about inserting the minimal number of calls. I've adjusted the code accordingly.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:865
+        // To check this we also need to nuke any dead constant uses (perhaps
+        // made dead by this operation on other functions).
+        Callee.removeDeadConstantUsers();
----------------
sanjoy wrote:
> This bit needs a unit test -- nothing in `make check` failed when I commented this out.
Yea, deleting the analogous line above also doesn't cause anything to fail.

:: sigh ::


================
Comment at: lib/Transforms/IPO/Inliner.cpp:865
+        // To check this we also need to nuke any dead constant uses (perhaps
+        // made dead by this operation on other functions).
+        Callee.removeDeadConstantUsers();
----------------
sanjoy wrote:
> This bit needs a unit test -- nothing in `make check` failed when I commented this out.
I've had to add numerous tests to cover all of the "last callsite" heuristics under this inliner. They include crafty test cases that cover this. I'm not really trying to make them work with the original inliner though as the approach used there is fundamentally harder to test.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:881
+    // callee.
+    for (Function *InlinedCallee : InlinedCallees) {
+      LazyCallGraph::Node &CalleeN = *CG.lookup(*InlinedCallee);
----------------
eraman wrote:
> 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. 
Yes, this is an over approximation. But we then clean all of these (dead) edges up with the below CG update routine.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:897
+    C = &updateCGAndAnalysisManagerForFunctionPass(CG, *C, N, AM, UR,
+                                                   /*DebugLogging*/ true);
+    RC = &C->getOuterRefSCC();
----------------
sanjoy wrote:
> Why is `DebugLogging` true by default?
.... Because I didn't thread an explicit DebugLogging flag through this code (and I don't want to), and I haven't taught the CGSCC update logic code to support a side channel like DEBUG_PASS yet, so I hard coded it.

Anyways, removed. The right way to make this stuff debuggable is to support DEBUG_PASS, but that's a separate issue and nothing to do with this patch.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1645
       UpdateCallGraphAfterInlining(CS, FirstNewBlock, VMap, IFI);
+    } else {
+      // Otherwise just collect the raw call sites that were inlined.
----------------
eraman wrote:
> 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?
I don't think this is really a big deal, and I'd rather not deal with ownership issues of a pointer....

The partial inlining code isn't really heavily used, and it remains correct. In fact, I suspect that if we actually wanted to keep the partial inlining code (but I don't think we do long-term), we'd want to use this exact data structure to iterate the same way we do in the inliner.

So if its OK, I'd rather keep this code simple and pay a (very small I think) overhead of building this when it isn't used in that context.


https://reviews.llvm.org/D24226