SILOptimizer: Fix analysis invalidation after devirtualization.

atrick · atrick · commit 70678ab856e2 · 2020-03-18T10:21:35.000-07:00
Devirtualizing try_apply modified the CFG, but passes that run
devirtualization were not invalidating any CFG analyses, such as the
domtree.

This could hypothetically have caused miscompilation, but will be
caught by running with -sil-verify-all.
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,
