[PATCH] D115545: [Inline] Attempt to delete any discardable if unused functions

Mircea Trofin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 10 11:23:37 PST 2022


mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:952
         Callee.removeDeadConstantUsers();
-        if (Callee.use_empty() && !CG.isLibFunction(Callee)) {
-          Calls->erase_if([&](const std::pair<CallBase *, int> &Call) {
-            return Call.first->getCaller() == &Callee;
-          });
-          // Clear the body and queue the function itself for deletion when we
-          // finish inlining and call graph updates.
-          // Note that after this point, it is an error to do anything other
-          // than use the callee's address or delete it.
-          Callee.dropAllReferences();
-          assert(!is_contained(DeadFunctions, &Callee) &&
-                 "Cannot put cause a function to become dead twice!");
-          DeadFunctions.push_back(&Callee);
-          CalleeWasDeleted = true;
+        if (Callee.use_empty() && !Callee.isUsedByMetadata() &&
+            !CG.isLibFunction(Callee)) {
----------------
mtrofin wrote:
> mtrofin wrote:
> > nikic wrote:
> > > I don't think this is the right way to fix it. The whole premise of metadata users is that they automatically get dropped when the value is destroyed.
> > > 
> > > I think the problem here is that for the comdat case, we call recordInlining() rather than recordInliningWithCalleeDeleted(), and apparently for some reason the InlineAdvisor is actually responsible for destroying functions, so the function ends up only getting removed from the module, but not erased, and metadata users never get dropped. This means that other cleanup also won't happen, not just the incorrect metadata uses.
> > > 
> > > I think we either need to restrict this to the case where the comdat is trivial (only has one entry) and we can make the decision immediately, or we need to adjust the advisor API to separate out "recordInlining" from "recordCalleeDeleted". Not sure how that would interact with the ML inline advisor though, cc @mtrofin.
> > > 
> > > I'd recommend to go for the trivial comdat check for now, because I think that's the main interesting case anyway.
> > > I don't think this is the right way to fix it. The whole premise of metadata users is that they automatically get dropped when the value is destroyed.
> > > 
> > > I think the problem here is that for the comdat case, we call recordInlining() rather than recordInliningWithCalleeDeleted(), and apparently for some reason the InlineAdvisor is actually responsible for destroying functions, so the function ends up only getting removed from the module, but not erased, and metadata users never get dropped. This means that other cleanup also won't happen, not just the incorrect metadata uses.
> > > 
> > > I think we either need to restrict this to the case where the comdat is trivial (only has one entry) and we can make the decision immediately, or we need to adjust the advisor API to separate out "recordInlining" from "recordCalleeDeleted". Not sure how that would interact with the ML inline advisor though, cc @mtrofin.
> > > 
> > > I'd recommend to go for the trivial comdat check for now, because I think that's the main interesting case anyway.
> > 
> > 
> The reason the advisor took the responsibility of actually deleting (i.e. `delete` - deleting) dead functions was to allow it to use function pointers to track some module-wide state. So then all other cleanup should have been done here, but the actual object is deleted at the end. We missed the cleanup in GlobalObject.
> 
> I think the better approach is to use Node (LazySCC::Node). I think I should do that change, remove the deleting responsibility from the Advisor, then we can delete functions at line 1048 down, and then, like @nikic observed, we should call here `recordInliningWithCaleeDeleted`, because we want to track if the effect of an inlining simplifies the call graph. If that sounds reasonable, I'll have the patch up shortly.
> 
> BTW - so if not all linkonce_odr functions can be deleted, then what happens, they'd still be emitted, but then, at link time, they'd be dropped?
D116964 for removing the deleting responsibility from the Advisor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115545/new/

https://reviews.llvm.org/D115545



More information about the llvm-commits mailing list