Merge pull request swiftlang#30473 from atrick/fix-devirtualizer-invalidate

atrick · web-flow · commit d28ab085cc5a · 2020-03-18T13:21:19.000-07:00
SILOptimizer: Fix analysis invalidation after devirtualization.
diff --git a/include/swift/SILOptimizer/Utils/Devirtualize.h b/include/swift/SILOptimizer/Utils/Devirtualize.h
@@ -65,9 +65,11 @@ SubstitutionMap getWitnessMethodSubstitutions(SILModule &Module, ApplySite AI,
 ///
 /// If this succeeds, the caller must call deleteDevirtualizedApply on
 /// the original apply site.
-ApplySite tryDevirtualizeApply(ApplySite AI,
-                               ClassHierarchyAnalysis *CHA,
-                               OptRemark::Emitter *ORE = nullptr);
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<ApplySite, bool>
+tryDevirtualizeApply(ApplySite AI, ClassHierarchyAnalysis *CHA,
+                     OptRemark::Emitter *ORE = nullptr);
 bool canDevirtualizeApply(FullApplySite AI, ClassHierarchyAnalysis *CHA);
 bool canDevirtualizeClassMethod(FullApplySite AI, ClassDecl *CD,
                                 OptRemark::Emitter *ORE = nullptr,
@@ -79,21 +81,23 @@ CanType getSelfInstanceType(CanType ClassOrMetatypeType);
 /// Devirtualize the given apply site, which is known to be devirtualizable.
 ///
 /// The caller must call deleteDevirtualizedApply on the original apply site.
-FullApplySite devirtualizeClassMethod(FullApplySite AI,
-                                      SILValue ClassInstance,
-                                      ClassDecl *CD,
-                                      OptRemark::Emitter *ORE);
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<FullApplySite, bool> devirtualizeClassMethod(FullApplySite AI,
+                                                       SILValue ClassInstance,
+                                                       ClassDecl *CD,
+                                                       OptRemark::Emitter *ORE);
 
 /// Attempt to devirtualize the given apply site, which is known to be
 /// of a class method.  If this fails, the returned FullApplySite will be null.
 ///
 /// If this succeeds, the caller must call deleteDevirtualizedApply on
 /// the original apply site.
-FullApplySite
-tryDevirtualizeClassMethod(FullApplySite AI,
-                           SILValue ClassInstance,
-                           ClassDecl *CD,
-                           OptRemark::Emitter *ORE,
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<FullApplySite, bool>
+tryDevirtualizeClassMethod(FullApplySite AI, SILValue ClassInstance,
+                           ClassDecl *CD, OptRemark::Emitter *ORE,
                            bool isEffectivelyFinalMethod = false);
 
 /// Attempt to devirtualize the given apply site, which is known to be
@@ -102,7 +106,9 @@ tryDevirtualizeClassMethod(FullApplySite AI,
 ///
 /// If this succeeds, the caller must call deleteDevirtualizedApply on
 /// the original apply site.
-ApplySite tryDevirtualizeWitnessMethod(ApplySite AI, OptRemark::Emitter *ORE);
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<ApplySite, bool> tryDevirtualizeWitnessMethod(ApplySite AI, OptRemark::Emitter *ORE);
 
 /// Delete a successfully-devirtualized apply site.  This must always be
 /// called after devirtualizing an apply; not only is it not semantically
diff --git a/lib/SILOptimizer/Mandatory/MandatoryInlining.cpp b/lib/SILOptimizer/Mandatory/MandatoryInlining.cpp
@@ -760,7 +760,7 @@ getCalleeFunction(SILFunction *F, FullApplySite AI, bool &IsThick,
 
 static SILInstruction *tryDevirtualizeApplyHelper(FullApplySite InnerAI,
                                                   ClassHierarchyAnalysis *CHA) {
-  auto NewInst = tryDevirtualizeApply(InnerAI, CHA);
+  auto NewInst = tryDevirtualizeApply(InnerAI, CHA).first;
   if (!NewInst)
     return InnerAI.getInstruction();
 
diff --git a/lib/SILOptimizer/Transforms/Devirtualizer.cpp b/lib/SILOptimizer/Transforms/Devirtualizer.cpp
@@ -30,8 +30,10 @@ using namespace swift;
 namespace {
 
 class Devirtualizer : public SILFunctionTransform {
+  bool Changed = false;
+  bool ChangedCFG = false;
 
-  bool devirtualizeAppliesInFunction(SILFunction &F,
+  void devirtualizeAppliesInFunction(SILFunction &F,
                                      ClassHierarchyAnalysis *CHA);
 
   /// The entry point to the transformation.
@@ -41,17 +43,22 @@ class Devirtualizer : public SILFunctionTransform {
     LLVM_DEBUG(llvm::dbgs() << "***** Devirtualizer on function:" << F.getName()
                             << " *****\n");
 
-    if (devirtualizeAppliesInFunction(F, CHA))
+    Changed = false;
+    ChangedCFG = false;
+    devirtualizeAppliesInFunction(F, CHA);
+    if (ChangedCFG)
+      invalidateAnalysis(SILAnalysis::InvalidationKind::Everything);
+    else if (Changed)
       invalidateAnalysis(SILAnalysis::InvalidationKind::CallsAndInstructions);
   }
 
 };
 
 } // end anonymous namespace
 
-bool Devirtualizer::devirtualizeAppliesInFunction(SILFunction &F,
+// Return true if any calls changed, and true if the CFG also changed.
+void Devirtualizer::devirtualizeAppliesInFunction(SILFunction &F,
                                                   ClassHierarchyAnalysis *CHA) {
-  bool Changed = false;
   llvm::SmallVector<ApplySite, 8> NewApplies;
   OptRemark::Emitter ORE(DEBUG_TYPE, F.getModule());
 
@@ -69,11 +76,14 @@ bool Devirtualizer::devirtualizeAppliesInFunction(SILFunction &F,
    }
   }
   for (auto Apply : Applies) {
-    auto NewInst = tryDevirtualizeApply(Apply, CHA, &ORE);
+    ApplySite NewInst;
+    bool modifiedCFG;
+    std::tie(NewInst, modifiedCFG) = tryDevirtualizeApply(Apply, CHA, &ORE);
     if (!NewInst)
       continue;
 
     Changed = true;
+    ChangedCFG |= modifiedCFG;
 
     deleteDevirtualizedApply(Apply);
     NewApplies.push_back(NewInst);
@@ -105,8 +115,6 @@ bool Devirtualizer::devirtualizeAppliesInFunction(SILFunction &F,
     if (CalleeFn->isDefinition() && CalleeFn->shouldOptimize())
       addFunctionToPassManagerWorklist(CalleeFn, nullptr);
   }
-
-  return Changed;
 }
 
 SILTransform *swift::createDevirtualizer() { return new Devirtualizer(); }
diff --git a/lib/SILOptimizer/Transforms/SpeculativeDevirtualizer.cpp b/lib/SILOptimizer/Transforms/SpeculativeDevirtualizer.cpp
@@ -229,7 +229,8 @@ static FullApplySite speculateMonomorphicTarget(FullApplySite AI,
 
   // Devirtualize the apply instruction on the identical path.
   auto NewInst =
-    devirtualizeClassMethod(IdenAI, DownCastedClassInstance, CD, nullptr);
+      devirtualizeClassMethod(IdenAI, DownCastedClassInstance, CD, nullptr)
+          .first;
   assert(NewInst && "Expected to be able to devirtualize apply!");
   (void)NewInst;
 
@@ -414,7 +415,8 @@ static bool tryToSpeculateTarget(FullApplySite AI, ClassHierarchyAnalysis *CHA,
     // try to devirtualize it completely.
     ClassHierarchyAnalysis::ClassList Subs;
     if (isDefaultCaseKnown(CHA, AI, CD, Subs)) {
-      auto NewInst = tryDevirtualizeClassMethod(AI, SubTypeValue, CD, &ORE);
+      auto NewInst =
+          tryDevirtualizeClassMethod(AI, SubTypeValue, CD, &ORE).first;
       if (NewInst)
         deleteDevirtualizedApply(AI);
       return bool(NewInst);
@@ -574,7 +576,8 @@ static bool tryToSpeculateTarget(FullApplySite AI, ClassHierarchyAnalysis *CHA,
     ORE.emit(RB);
     return true;
   }
-  auto NewInst = tryDevirtualizeClassMethod(AI, SubTypeValue, CD, nullptr);
+  auto NewInst =
+      tryDevirtualizeClassMethod(AI, SubTypeValue, CD, nullptr).first;
   if (NewInst) {
     ORE.emit(RB);
     deleteDevirtualizedApply(AI);
diff --git a/lib/SILOptimizer/Utils/Devirtualize.cpp b/lib/SILOptimizer/Utils/Devirtualize.cpp
@@ -482,12 +482,12 @@ static ApplyInst *replaceApplyInst(SILBuilder &builder, SILLocation loc,
   return newAI;
 }
 
-static TryApplyInst *replaceTryApplyInst(SILBuilder &builder, SILLocation loc,
-                                         TryApplyInst *oldTAI, SILValue newFn,
-                                         SubstitutionMap newSubs,
-                                         ArrayRef<SILValue> newArgs,
-                                         SILFunctionConventions conv,
-                                         ArrayRef<SILValue> newArgBorrows) {
+// Return the new try_apply and true if a cast required CFG modification.
+static std::pair<TryApplyInst *, bool>
+replaceTryApplyInst(SILBuilder &builder, SILLocation loc, TryApplyInst *oldTAI,
+                    SILValue newFn, SubstitutionMap newSubs,
+                    ArrayRef<SILValue> newArgs, SILFunctionConventions conv,
+                    ArrayRef<SILValue> newArgBorrows) {
   SILBasicBlock *normalBB = oldTAI->getNormalBB();
   SILBasicBlock *resultBB = nullptr;
 
@@ -537,7 +537,7 @@ static TryApplyInst *replaceTryApplyInst(SILBuilder &builder, SILLocation loc,
   }
 
   builder.setInsertionPoint(normalBB->begin());
-  return newTAI;
+  return {newTAI, resultCastRequired};
 }
 
 static BeginApplyInst *
@@ -599,17 +599,18 @@ replacePartialApplyInst(SILBuilder &builder, SILLocation loc,
   return newPAI;
 }
 
-static ApplySite replaceApplySite(SILBuilder &builder, SILLocation loc,
-                                  ApplySite oldAS, SILValue newFn,
-                                  SubstitutionMap newSubs,
-                                  ArrayRef<SILValue> newArgs,
-                                  SILFunctionConventions conv,
-                                  ArrayRef<SILValue> newArgBorrows) {
+// Return the new apply and true if the CFG was also modified.
+static std::pair<ApplySite, bool>
+replaceApplySite(SILBuilder &builder, SILLocation loc, ApplySite oldAS,
+                 SILValue newFn, SubstitutionMap newSubs,
+                 ArrayRef<SILValue> newArgs, SILFunctionConventions conv,
+                 ArrayRef<SILValue> newArgBorrows) {
   switch (oldAS.getKind()) {
   case ApplySiteKind::ApplyInst: {
     auto *oldAI = cast<ApplyInst>(oldAS);
-    return replaceApplyInst(builder, loc, oldAI, newFn, newSubs, newArgs,
-                            newArgBorrows);
+    return {replaceApplyInst(builder, loc, oldAI, newFn, newSubs, newArgs,
+                             newArgBorrows),
+            false};
   }
   case ApplySiteKind::TryApplyInst: {
     auto *oldTAI = cast<TryApplyInst>(oldAS);
@@ -618,14 +619,16 @@ static ApplySite replaceApplySite(SILBuilder &builder, SILLocation loc,
   }
   case ApplySiteKind::BeginApplyInst: {
     auto *oldBAI = dyn_cast<BeginApplyInst>(oldAS);
-    return replaceBeginApplyInst(builder, loc, oldBAI, newFn, newSubs, newArgs,
-                                 newArgBorrows);
+    return {replaceBeginApplyInst(builder, loc, oldBAI, newFn, newSubs, newArgs,
+                                  newArgBorrows),
+            false};
   }
   case ApplySiteKind::PartialApplyInst: {
     assert(newArgBorrows.empty());
     auto *oldPAI = cast<PartialApplyInst>(oldAS);
-    return replacePartialApplyInst(builder, loc, oldPAI, newFn, newSubs,
-                                   newArgs);
+    return {
+        replacePartialApplyInst(builder, loc, oldPAI, newFn, newSubs, newArgs),
+        false};
   }
   }
   llvm_unreachable("covered switch");
@@ -729,10 +732,12 @@ bool swift::canDevirtualizeClassMethod(FullApplySite applySite, ClassDecl *cd,
 /// \p ClassOrMetatype is a class value or metatype value that is the
 ///    self argument of the apply we will devirtualize.
 /// return the result value of the new ApplyInst if created one or null.
-FullApplySite swift::devirtualizeClassMethod(FullApplySite applySite,
-                                             SILValue classOrMetatype,
-                                             ClassDecl *cd,
-                                             OptRemark::Emitter *ore) {
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<FullApplySite, bool>
+swift::devirtualizeClassMethod(FullApplySite applySite,
+                               SILValue classOrMetatype, ClassDecl *cd,
+                               OptRemark::Emitter *ore) {
   LLVM_DEBUG(llvm::dbgs() << "    Trying to devirtualize : "
                           << *applySite.getInstruction());
 
@@ -793,8 +798,10 @@ FullApplySite swift::devirtualizeClassMethod(FullApplySite applySite,
     newArgs.push_back(arg);
     ++paramArgIter;
   }
-  ApplySite newAS = replaceApplySite(builder, loc, applySite, fri, subs,
-                                     newArgs, substConv, newArgBorrows);
+  ApplySite newAS;
+  bool modifiedCFG;
+  std::tie(newAS, modifiedCFG) = replaceApplySite(
+      builder, loc, applySite, fri, subs, newArgs, substConv, newArgBorrows);
   FullApplySite newAI = FullApplySite::isa(newAS.getInstruction());
   assert(newAI);
 
@@ -808,16 +815,14 @@ FullApplySite swift::devirtualizeClassMethod(FullApplySite applySite,
     });
   NumClassDevirt++;
 
-  return newAI;
+  return {newAI, modifiedCFG};
 }
 
-FullApplySite swift::tryDevirtualizeClassMethod(FullApplySite applySite,
-                                                SILValue classInstance,
-                                                ClassDecl *cd,
-                                                OptRemark::Emitter *ore,
-                                                bool isEffectivelyFinalMethod) {
+std::pair<FullApplySite, bool> swift::tryDevirtualizeClassMethod(
+    FullApplySite applySite, SILValue classInstance, ClassDecl *cd,
+    OptRemark::Emitter *ore, bool isEffectivelyFinalMethod) {
   if (!canDevirtualizeClassMethod(applySite, cd, ore, isEffectivelyFinalMethod))
-    return FullApplySite();
+    return {FullApplySite(), false};
   return devirtualizeClassMethod(applySite, classInstance, cd, ore);
 }
 
@@ -960,9 +965,12 @@ swift::getWitnessMethodSubstitutions(SILModule &module, ApplySite applySite,
 /// Generate a new apply of a function_ref to replace an apply of a
 /// witness_method when we've determined the actual function we'll end
 /// up calling.
-static ApplySite devirtualizeWitnessMethod(ApplySite applySite, SILFunction *f,
-                                           ProtocolConformanceRef cRef,
-                                           OptRemark::Emitter *ore) {
+///
+/// Return the new apply and true if the CFG was also modified.
+static std::pair<ApplySite, bool>
+devirtualizeWitnessMethod(ApplySite applySite, SILFunction *f,
+                          ProtocolConformanceRef cRef,
+                          OptRemark::Emitter *ore) {
   // We know the witness thunk and the corresponding set of substitutions
   // required to invoke the protocol method at this point.
   auto &module = applySite.getModule();
@@ -1017,7 +1025,9 @@ static ApplySite devirtualizeWitnessMethod(ApplySite applySite, SILFunction *f,
   SILLocation loc = applySite.getLoc();
   auto *fri = applyBuilder.createFunctionRefFor(loc, f);
 
-  ApplySite newApplySite =
+  ApplySite newApplySite;
+  bool modifiedCFG;
+  std::tie(newApplySite, modifiedCFG) =
       replaceApplySite(applyBuilder, loc, applySite, fri, subMap, arguments,
                        substConv, borrowedArgs);
 
@@ -1029,7 +1039,7 @@ static ApplySite devirtualizeWitnessMethod(ApplySite applySite, SILFunction *f,
              << "Devirtualized call to " << NV("Method", f);
     });
   NumWitnessDevirt++;
-  return newApplySite;
+  return {newApplySite, modifiedCFG};
 }
 
 static bool canDevirtualizeWitnessMethod(ApplySite applySite) {
@@ -1066,10 +1076,11 @@ static bool canDevirtualizeWitnessMethod(ApplySite applySite) {
 /// In the cases where we can statically determine the function that
 /// we'll call to, replace an apply of a witness_method with an apply
 /// of a function_ref, returning the new apply.
-ApplySite swift::tryDevirtualizeWitnessMethod(ApplySite applySite,
-                                              OptRemark::Emitter *ore) {
+std::pair<ApplySite, bool>
+swift::tryDevirtualizeWitnessMethod(ApplySite applySite,
+                                    OptRemark::Emitter *ore) {
   if (!canDevirtualizeWitnessMethod(applySite))
-    return ApplySite();
+    return {ApplySite(), false};
 
   SILFunction *f;
   SILWitnessTable *wt;
@@ -1088,9 +1099,11 @@ ApplySite swift::tryDevirtualizeWitnessMethod(ApplySite applySite,
 
 /// Attempt to devirtualize the given apply if possible, and return a
 /// new instruction in that case, or nullptr otherwise.
-ApplySite swift::tryDevirtualizeApply(ApplySite applySite,
-                                      ClassHierarchyAnalysis *cha,
-                                      OptRemark::Emitter *ore) {
+///
+/// Return the new apply and true if the CFG was also modified.
+std::pair<ApplySite, bool>
+swift::tryDevirtualizeApply(ApplySite applySite, ClassHierarchyAnalysis *cha,
+                            OptRemark::Emitter *ore) {
   LLVM_DEBUG(llvm::dbgs() << "    Trying to devirtualize: "
                           << *applySite.getInstruction());
 
@@ -1105,7 +1118,7 @@ ApplySite swift::tryDevirtualizeApply(ApplySite applySite,
   // TODO: check if we can also de-virtualize partial applies of class methods.
   FullApplySite fas = FullApplySite::isa(applySite.getInstruction());
   if (!fas)
-    return ApplySite();
+    return {ApplySite(), false};
 
   /// Optimize a class_method and alloc_ref pair into a direct function
   /// reference:
@@ -1151,7 +1164,7 @@ ApplySite swift::tryDevirtualizeApply(ApplySite applySite,
     return tryDevirtualizeClassMethod(fas, instance, cd, ore);
   }
 
-  return ApplySite();
+  return {ApplySite(), false};
 }
 
 bool swift::canDevirtualizeApply(FullApplySite applySite,
