[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 inline comments.


================
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:
> 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.  


https://reviews.llvm.org/D24226