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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a subscriber: mtrofin.
nikic 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)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115545