Merge pull request #28484 from zoecarver/optimize/mandatory-function-cleanup-pass

Remove dead function cleanup from mandatory inlining

Author: zoecarver

lib/SILOptimizer/Mandatory/MandatoryInlining.cpp

@@ -822,12 +822,12 @@ static SILInstruction *tryDevirtualizeApplyHelper(FullApplySite InnerAI,
 ///
 /// \returns true if successful, false if failed due to circular inlining.
 static bool
-runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder,
-			 SILFunction *F, FullApplySite AI,
-                         DenseFunctionSet &FullyInlinedSet,
+runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder, SILFunction *F,
+                         FullApplySite AI, DenseFunctionSet &FullyInlinedSet,
                          ImmutableFunctionSet::Factory &SetFactory,
                          ImmutableFunctionSet CurrentInliningSet,
-                         ClassHierarchyAnalysis *CHA) {
+                         ClassHierarchyAnalysis *CHA,
+                         DenseFunctionSet &changedFunctions) {
   // Avoid reprocessing functions needlessly.
   if (FullyInlinedSet.count(F))
     return true;
@@ -874,6 +874,10 @@ runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder,
       // but a casted result of InnerAI or even a block argument due to
       // abstraction changes when calling the witness or class method.
       auto *devirtInst = tryDevirtualizeApplyHelper(InnerAI, CHA);
+      // If devirtualization succeeds, make sure we record that this function
+      // changed.
+      if (devirtInst != InnerAI.getInstruction())
+        changedFunctions.insert(F);
       // Restore II to the current apply site.
       II = devirtInst->getReverseIterator();
       // If the devirtualized call result is no longer a invalid FullApplySite,
@@ -892,9 +896,9 @@ runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder,
         continue;
 
       // Then recursively process it first before trying to inline it.
-      if (!runOnFunctionRecursively(FuncBuilder, CalleeFunction, InnerAI,
-                                    FullyInlinedSet, SetFactory,
-                                    CurrentInliningSet, CHA)) {
+      if (!runOnFunctionRecursively(
+              FuncBuilder, CalleeFunction, InnerAI, FullyInlinedSet, SetFactory,
+              CurrentInliningSet, CHA, changedFunctions)) {
         // If we failed due to circular inlining, then emit some notes to
         // trace back the failure if we have more information.
         // FIXME: possibly it could be worth recovering and attempting other
@@ -984,6 +988,10 @@ runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder,
       closureCleanup.cleanupDeadClosures(F);
       invalidatedStackNesting |= closureCleanup.invalidatedStackNesting;
 
+      // Record that we inlined into this function so that we can invalidate it
+      // later.
+      changedFunctions.insert(F);
+
       // Resume inlining within nextBB, which contains only the inlined
       // instructions and possibly instructions in the original call block that
       // have not yet been visited.
@@ -993,6 +1001,7 @@ runOnFunctionRecursively(SILOptFunctionBuilder &FuncBuilder,
 
   if (invalidatedStackNesting) {
     StackNesting().correctStackNesting(F);
+    changedFunctions.insert(F);
   }
 
   // Keep track of full inlined functions so we don't waste time recursively
@@ -1012,10 +1021,10 @@ class MandatoryInlining : public SILModuleTransform {
   void run() override {
     ClassHierarchyAnalysis *CHA = getAnalysis<ClassHierarchyAnalysis>();
     SILModule *M = getModule();
-    bool ShouldCleanup = !getOptions().DebugSerialization;
     bool SILVerifyAll = getOptions().VerifyAll;
     DenseFunctionSet FullyInlinedSet;
     ImmutableFunctionSet::Factory SetFactory;
+    DenseFunctionSet changedFunctions;
 
     SILOptFunctionBuilder FuncBuilder(*this);
     for (auto &F : *M) {
@@ -1027,13 +1036,15 @@ class MandatoryInlining : public SILModuleTransform {
       if (F.wasDeserializedCanonical())
         continue;
 
-      runOnFunctionRecursively(FuncBuilder, &F,
-                               FullApplySite(), FullyInlinedSet, SetFactory,
-                               SetFactory.getEmptySet(), CHA);
+      runOnFunctionRecursively(FuncBuilder, &F, FullApplySite(),
+                               FullyInlinedSet, SetFactory,
+                               SetFactory.getEmptySet(), CHA, changedFunctions);
 
       // The inliner splits blocks at call sites. Re-merge trivial branches
       // to reestablish a canonical CFG.
-      mergeBasicBlocks(&F);
+      if (mergeBasicBlocks(&F)) {
+        changedFunctions.insert(&F);
+      }
 
       // If we are asked to perform SIL verify all, perform that now so that we
       // can discover the immediate inlining trigger of the problematic
@@ -1043,38 +1054,10 @@ class MandatoryInlining : public SILModuleTransform {
       }
     }
 
-    if (!ShouldCleanup)
+    if (getOptions().DebugSerialization)
       return;
-
-    // Now that we've inlined some functions, clean up.  If there are any
-    // transparent functions that are deserialized from another module that are
-    // now unused, just remove them from the module.
-    //
-    // We do this with a simple linear scan, because transparent functions that
-    // reference each other have already been flattened.
-    for (auto FI = M->begin(), E = M->end(); FI != E; ) {
-      SILFunction &F = *FI++;
-
-      invalidateAnalysis(&F, SILAnalysis::InvalidationKind::Everything);
-
-      if (F.getRefCount() != 0) continue;
-
-      // Leave non-transparent functions alone.
-      if (!F.isTransparent())
-        continue;
-
-      // We discard functions that don't have external linkage,
-      // e.g. deserialized functions, internal functions, and thunks.
-      // Being marked transparent controls this.
-      if (F.isPossiblyUsedExternally()) continue;
-
-      // ObjC functions are called through the runtime and are therefore alive
-      // even if not referenced inside SIL.
-      if (F.getRepresentation() == SILFunctionTypeRepresentation::ObjCMethod)
-        continue;
-
-      // Okay, just erase the function from the module.
-      FuncBuilder.eraseFunction(&F);
+    for (auto *F : changedFunctions) {
+      invalidateAnalysis(F, SILAnalysis::InvalidationKind::Everything);
     }
   }
 

test/SILOptimizer/mandatory_inlining.sil

@@ -960,9 +960,7 @@ bb0(%0 : $Builtin.Int8):
   return %5 : $Builtin.Int8
 }
 
-// We delete this now.
-//
-// CHECK-NOT: sil shared [transparent] @identity_closure
+// This isn't removed in Onone anymore.
 sil shared [transparent] @identity_closure : $@convention(thin) (Builtin.Int8) -> Builtin.Int8 {
 bb0(%0 : $Builtin.Int8):
   return %0 : $Builtin.Int8

validation-test/SILOptimizer/mandatory_inlining_gets_invalidated.sil

@@ -0,0 +1,35 @@
+// RUN: %target-sil-opt -compute-dominance-info -mandatory-inlining -enable-sil-verify-all %s
+
+// This test ensures that functions which are inlined into also get invalidated
+// with invalidateAnalysis.
+
+import Builtin
+import Swift
+
+sil @f : $@convention(thin) () -> () {
+bb0:
+  // function_ref g()
+  %0 = function_ref @g : $@convention(thin) () -> ()
+  %1 = apply %0() : $@convention(thin) () -> ()
+  %2 = tuple ()
+  return %2 : $()
+}
+
+sil [transparent] @h : $@convention(thin) () -> Builtin.Int1 {
+bb0:
+  %0 = integer_literal $Builtin.Int1, 0
+  br bb2
+bb1:
+  return %0 : $Builtin.Int1
+bb2:
+  br bb1
+}
+
+sil [transparent] @g : $@convention(thin) () -> () {
+bb0:
+  // function_ref h()
+  %0 = function_ref @h : $@convention(thin) () -> Builtin.Int1
+  %1 = apply %0() : $@convention(thin) () -> Builtin.Int1
+  %2 = tuple ()
+  return %2 : $()
+}