Merge pull request #13102 from slavapestov/unbound-generic-type-mess

Fix speculative devirtualizer issue with nested generic classes

Author: slavapestov

include/swift/SILOptimizer/Utils/Devirtualize.h

@@ -48,6 +48,19 @@ namespace swift {
 /// between optional types.
 typedef std::pair<ValueBase *, ApplySite> DevirtualizationResult;
 
+/// Compute all subclasses of a given class.
+///
+/// \p CHA class hierarchy analysis
+/// \p CD class declaration
+/// \p ClassType type of the instance
+/// \p M SILModule
+/// \p Subs a container to be used for storing the set of subclasses
+void getAllSubclasses(ClassHierarchyAnalysis *CHA,
+                      ClassDecl *CD,
+                      SILType ClassType,
+                      SILModule &M,
+                      ClassHierarchyAnalysis::ClassList &Subs);
+
 DevirtualizationResult tryDevirtualizeApply(ApplySite AI,
                                             ClassHierarchyAnalysis *CHA);
 bool canDevirtualizeApply(FullApplySite AI, ClassHierarchyAnalysis *CHA);

lib/AST/Type.cpp

@@ -433,6 +433,9 @@ static bool isLegalSILType(CanType type) {
   // L-values and inouts are not legal.
   if (!type->isMaterializable()) return false;
 
+  // Unbound generic types are not legal.
+  if (type->hasUnboundGenericType()) return false;
+
   // Function types must be lowered.
   if (isa<AnyFunctionType>(type)) return false;
 

lib/IRGen/GenMeta.cpp

@@ -133,8 +133,6 @@ namespace {
   /// nominal metadata reference.  The structure produced here is
   /// consumed by swift_getGenericMetadata() and must correspond to
   /// the fill operations that the compiler emits for the bound decl.
-  ///
-  /// FIXME: Rework to use GenericSignature instead of AllArchetypes
   struct GenericArguments {
     /// The values to use to initialize the arguments structure.
     SmallVector<llvm::Value *, 8> Values;

lib/SILOptimizer/Transforms/SpeculativeDevirtualizer.cpp

@@ -378,38 +378,8 @@ static bool tryToSpeculateTarget(FullApplySite AI,
   // True if any instructions were changed or generated.
   bool Changed = false;
 
-  // Collect the direct and indirect subclasses for the class.
-  // Sort these subclasses in the order they should be tested by the
-  // speculative devirtualization. Different strategies could be used,
-  // E.g. breadth-first, depth-first, etc.
-  // Currently, let's use the breadth-first strategy.
-  // The exact static type of the instance should be tested first.
-  auto &DirectSubs = CHA->getDirectSubClasses(CD);
-  auto &IndirectSubs = CHA->getIndirectSubClasses(CD);
-
-  SmallVector<ClassDecl *, 8> Subs(DirectSubs);
-  Subs.append(IndirectSubs.begin(), IndirectSubs.end());
-
-  if (ClassType.is<BoundGenericClassType>()) {
-    // Filter out any subclasses that do not inherit from this
-    // specific bound class.
-    auto RemovedIt = std::remove_if(Subs.begin(),
-        Subs.end(),
-        [&ClassType](ClassDecl *Sub){
-          auto SubCanTy = Sub->getDeclaredType()->getCanonicalType();
-          // Unbound generic type can override a method from
-          // a bound generic class, but this unbound generic
-          // class is not considered to be a subclass of a
-          // bound generic class in a general case.
-          if (isa<UnboundGenericType>(SubCanTy))
-            return false;
-          // Handle the usual case here: the class in question
-          // should be a real subclass of a bound generic class.
-          return !ClassType.isBindableToSuperclassOf(
-              SILType::getPrimitiveObjectType(SubCanTy));
-        });
-    Subs.erase(RemovedIt, Subs.end());
-  }
+  SmallVector<ClassDecl *, 8> Subs;
+  getAllSubclasses(CHA, CD, ClassType, M, Subs);
 
   // Number of subclasses which cannot be handled by checked_cast_br checks.
   int NotHandledSubsNum = 0;
@@ -484,15 +454,15 @@ static bool tryToSpeculateTarget(FullApplySite AI,
     DEBUG(llvm::dbgs() << "Inserting a speculative call for class "
           << CD->getName() << " and subclass " << S->getName() << "\n");
 
-    CanType CanClassType = S->getDeclaredType()->getCanonicalType();
-    SILType ClassType = SILType::getPrimitiveObjectType(CanClassType);
-    if (!ClassType.getClassOrBoundGenericClass()) {
-      // This subclass cannot be handled. This happens e.g. if it is
-      // a generic class.
+    // FIXME: Add support for generic subclasses.
+    if (S->isGenericContext()) {
       NotHandledSubsNum++;
       continue;
     }
 
+    CanType CanClassType = S->getDeclaredInterfaceType()->getCanonicalType();
+    SILType ClassType = SILType::getPrimitiveObjectType(CanClassType);
+
     auto ClassOrMetatypeType = ClassType;
     if (auto EMT = SubType.getAs<AnyMetatypeType>()) {
       auto InstTy = ClassType.getSwiftRValueType();

lib/SILOptimizer/Utils/Devirtualize.cpp

@@ -39,14 +39,7 @@ STATISTIC(NumWitnessDevirt, "Number of witness_method applies devirtualized");
 //                         Class Method Optimization
 //===----------------------------------------------------------------------===//
 
-/// Compute all subclasses of a given class.
-///
-/// \p CHA class hierarchy analysis
-/// \p CD class declaration
-/// \p ClassType type of the instance
-/// \p M SILModule
-/// \p Subs a container to be used for storing the set of subclasses
-static void getAllSubclasses(ClassHierarchyAnalysis *CHA,
+void swift::getAllSubclasses(ClassHierarchyAnalysis *CHA,
                              ClassDecl *CD,
                              SILType ClassType,
                              SILModule &M,
@@ -61,21 +54,17 @@ static void getAllSubclasses(ClassHierarchyAnalysis *CHA,
   auto &IndirectSubs = CHA->getIndirectSubClasses(CD);
 
   Subs.append(DirectSubs.begin(), DirectSubs.end());
-  //SmallVector<ClassDecl *, 8> Subs(DirectSubs);
   Subs.append(IndirectSubs.begin(), IndirectSubs.end());
 
   if (ClassType.is<BoundGenericClassType>()) {
     // Filter out any subclasses that do not inherit from this
     // specific bound class.
     auto RemovedIt = std::remove_if(Subs.begin(), Subs.end(),
         [&ClassType](ClassDecl *Sub){
-          auto SubCanTy = Sub->getDeclaredType()->getCanonicalType();
-          // Unbound generic type can override a method from
-          // a bound generic class, but this unbound generic
-          // class is not considered to be a subclass of a
-          // bound generic class in a general case.
-          if (isa<UnboundGenericType>(SubCanTy))
+          // FIXME: Add support for generic subclasses.
+          if (Sub->isGenericContext())
             return false;
+          auto SubCanTy = Sub->getDeclaredInterfaceType()->getCanonicalType();
           // Handle the usual case here: the class in question
           // should be a real subclass of a bound generic class.
           return !ClassType.isBindableToSuperclassOf(
@@ -94,10 +83,10 @@ static void getAllSubclasses(ClassHierarchyAnalysis *CHA,
 /// \p ClassType type of the instance
 /// \p CD  static class of the instance whose method is being invoked
 /// \p CHA class hierarchy analysis
-bool isEffectivelyFinalMethod(FullApplySite AI,
-                              SILType ClassType,
-                              ClassDecl *CD,
-                              ClassHierarchyAnalysis *CHA) {
+static bool isEffectivelyFinalMethod(FullApplySite AI,
+                                     SILType ClassType,
+                                     ClassDecl *CD,
+                                     ClassHierarchyAnalysis *CHA) {
   if (CD && CD->isFinal())
     return true;
 

test/SILOptimizer/devirt_speculative_nested.swift

@@ -0,0 +1,31 @@
+// RUN: %target-swift-frontend %s -parse-as-library -O -emit-sil | %FileCheck %s
+// RUN: %target-swift-frontend %s -parse-as-library -Osize -emit-sil
+//
+// Test speculative devirtualization.
+
+public class Outer<T> {
+  final class Inner : Base {
+    @inline(never) override func update() {
+    }
+  }
+}
+
+public class Base {
+  @inline(never) func update() { }
+}
+
+// FIXME: We don't speculate to the override Outer<T>.Inner.update() here,
+// because we cannot express the cast -- the cast "returns" a new archetype
+// T, much like an opened existential.
+//
+// But at least, we shouldn't crash.
+
+// CHECK-LABEL: sil [thunk] [always_inline] @_T025devirt_speculative_nested3fooyAA4BaseC1x_tF : $@convention(thin) (@owned Base) -> ()
+// CHECK: checked_cast_br [exact] %0 : $Base to $Base
+// CHECK: function_ref @_T025devirt_speculative_nested4BaseC6updateyyF
+// CHECK: class_method %0 : $Base, #Base.update!1
+// CHECK: return
+
+public func foo(x: Base) {
+  x.update()
+}