Improve sil_combiner using subtyping information from CHA #16739

rajbarik · 2018-05-19T06:15:03Z

When sil_combiner can not determine the concrete type via "findInitExistential", this PR allows it to fall back to the sub-typing information available in the class hierarchy analysis to see if it can determine concrete type of the self argument. It then devirtualizes the apply instruction using this concrete type. This seems to be a generally useful optimization for apps that use protocols heavily -- including protocols used for test-mocking.

Example:

internal protocol SomeProtocol : class {
  func foo(x:SomeProtocol)  -> Int
  func foo_internal()  -> Int
}
internal class SomeClass: SomeProtocol {
  func foo_internal() ->Int {
    return 10
  }
  func foo(x:SomeProtocol) -> Int {
    return x.foo_internal()
  }
}
internal protocol SomeNonClassProtocol {
  func bar(x:SomeNonClassProtocol)  -> Int
  func bar_internal()  -> Int
}
internal class SomeNonClass: SomeNonClassProtocol {
  func bar_internal() -> Int {
    return 20
  }
  func bar(x:SomeNonClassProtocol) -> Int {
    return x.bar_internal()
  }
}
internal protocol UnrelatedProtocol {
  func foo()  -> Int
}
internal class SomeUnrelatedClass: UnrelatedProtocol {
  func foo() -> Int {
    return 20
  }
}
internal class SomeUnrelatedClassDerived: SomeUnrelatedClass {
}
internal class Other {
   let x:SomeProtocol
   let y:SomeNonClassProtocol
   let z:UnrelatedProtocol
   init(x:SomeProtocol, y:SomeNonClassProtocol, z:UnrelatedProtocol) {
     self.x = x; self.y = y; self.z = z;
   }
   **@inline(never) func doWork () ->Int {
      return self.x.foo(x:self.x) + self.y.bar(x:self.y) + self.z.foo()
   }**
}

If you look at method doWork(), the calls to self.x.foo, self.y.bar and self.z.foo are all witness methods. With subtyping information in class hierarchy, you can determine that self.x.foo and self.y.bar can be devirtualized. Note that self.z.foo can not be devirtualized since self.z can either be the base class SomeUnrelatedClass or the derived class SomeUnrelatedClassDerived.

rajbarik · 2018-05-19T06:17:34Z

@slavapestov when you get some free time, please have a look at this PR as well. I plan on combining this PR with my old PR from #13991.

slavapestov · 2018-05-19T07:59:34Z

This PR brings back the protocol implementation analysis that I removed in 1a66c89. The problem was that this analysis was performing a walk over all declarations in the module, once per source file. This meant quadratic complexity in non-WMO mode. Can you think of another way of achieving what you're trying to do here without the compile time impact?

gottesmm · 2018-05-19T18:02:17Z

@rajbarik just a friendly FYI. You can use 3 '`' before/after a block to make it render in fixed point. It makes it much easier to read code and demarcates it cleanly. I edited your post above to add that. If you hit the edit button you will see the change I made.

rajbarik · 2018-05-21T17:30:09Z

@slavapestov I have some ideas -- will update the PR and resubmit.
Thanks a lot for formatting the code! @gottesmm.

gottesmm · 2018-05-21T17:38:53Z

@rajbarik Happy to help = ).

rajbarik · 2018-06-01T20:33:09Z

@slavapestov I have moved the expensive DeclWalk from ClassHierarchyAnalysis to ConcreteTypeAnalysis and guarded it with WMO. That is, ConcreteTypeAnalysis is performed only in WMO. I did not find any API that just builds conformance table for a single source file for non-WMO mode. If you have any pointers, let me know -- I can easily re-enable this optimization for fileprivate declarations.

rajbarik · 2018-06-07T17:57:08Z

@slavapestov As per our discussion, I have added support for enums and structs. Moreover, ClassHierarchyAnalysis is moved out of ConcreteTypeAnalysis. It also handles the bug mentioned in https://bugs.swift.org/browse/SR-7891. Let me know the next steps.

slavapestov · 2018-06-08T21:13:05Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  }
+
+  auto ElementType = NTD->getDeclaredType()->getCanonicalType();
+  if ((isa<ClassDecl>(NTD)) && (!(ElementType->getClassOrBoundGenericClass()))) {


No need for the extra parentheses. Not clear why this condition occurs, add a comment

slavapestov · 2018-06-08T21:13:23Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+    return false;
+  }
+
+  auto ElementType = NTD->getDeclaredType()->getCanonicalType();


Unnecessary call to getCanonicalType()

slavapestov · 2018-06-08T21:13:37Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  ClassDecl *CD = nullptr;
+  /// Check if the class has no subclasses: direct or indirect.
+  if ((CD = dyn_cast<ClassDecl>(NTD)) &&
+          ((CHA->hasKnownDirectSubclasses(CD)) ||


Indent so it lines up with the if and remove redundant parens

slavapestov · 2018-06-08T21:14:14Z

lib/SILOptimizer/Analysis/ConcreteTypeAnalysis.cpp

@@ -0,0 +1,98 @@
+//===--- ConcreteTypeAnalysis.cpp - Protocol to Class inheritance ---------===//


Word "Class" should not appear here, and maybe rename this to "ProtocolConfomanceAnalysis"

in general we use "X conforms to Y" instead of "X implements Y"

slavapestov · 2018-06-08T21:14:26Z

lib/SILOptimizer/Analysis/ConcreteTypeAnalysis.cpp

+void ConcreteTypeAnalysis::init() {
+
+  // We only do this in Whole-Module compilation mode.
+  if (!(M->isWholeModule()))


Extra parens

slavapestov · 2018-06-08T21:21:20Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+    SILType ConcreteSILType = SILType::getPrimitiveAddressType(ConcreteType);
+    auto *UACI =
+        Builder.createUncheckedAddrCast(OEA->getLoc(), OEA, ConcreteSILType);
+    ConformanceRef = M.getSwiftModule()->lookupConformance(


Conformance should already be stored inside the OpenExistentialAddrInst

slavapestov · 2018-06-08T21:21:58Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+    NewArg = URCI;
+  } else if (auto *OEA = dyn_cast<OpenExistentialAddrInst>(Arg)) {
+    ConcreteType = ElementType;
+    SILType ConcreteSILType = SILType::getPrimitiveAddressType(ConcreteType);


This will produce an invalid SIL type if its an Optional. You need to lower the SIL type; SILModule.Types.getLoweredType(ConcreteType)

slavapestov · 2018-06-08T21:23:58Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  }
+  if (auto *OER = dyn_cast<OpenExistentialRefInst>(Arg)) {
+    ConcreteType = ElementType;
+    SILType ConcreteSILType = M.Types.getLoweredType(ConcreteType);


This is correct

slavapestov · 2018-06-08T21:26:14Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  return true;
+}
+
+//  If class C is the only class conforming to a protocol P, then


This is no longer specific to classes

slavapestov · 2018-06-08T21:28:37Z

test/SILOptimizer/sil_combine_propagate_cta.swift

@@ -0,0 +1,139 @@
+// RUN: %target-swift-frontend %s -O -wmo -emit-sil -Xllvm -sil-disable-pass=DeadFunctionElimination | %FileCheck %s
+
+internal protocol SomeProtocol : class {


Please test:

structs

public protocol (make sure its not optimized)

class conforming to non-class-bound protocol

generic class/struct/enum cannot be optimized

also check this case:

struct Outer<T> { struct Inner : MyProto { ... } }

@slavapestov I have added tests for all of the above except two. Can you help?
(1) Can you remind me what you meant by structs? I already have struct test.
(2) For this case:
struct Outer {
struct Inner : MyProto { ... }
}
The M.Types.getLoweredType call crashes with "Bad base type". Do you have any recommendations on how to obtain lowered SILType for generic nested types like above? I am thinking this case can be handled..

FYI, I can also skip the optimization altogether by checking if the concrete type is an unbound generic type using "hasUnboundGenericType()".

slavapestov · 2018-06-08T21:32:09Z

Couple of other comments:

How about a separate PR with just the analysis, and then a second PR for the SIL combiner change?
Test nested types, local types (types inside functions)
What about existentials like P & Q?

gottesmm

Quick drive by comment.

gottesmm · 2018-06-09T06:01:54Z

include/swift/SILOptimizer/Analysis/ConcreteTypeAnalysis.h

@@ -0,0 +1,70 @@
+#ifndef SWIFT_SILOPTIMIZER_ANALYSIS_CONCRETETYPE_H


This needs a file header.

rajbarik · 2018-06-13T17:48:17Z

@slavapestov Smaller PR with just the analysis is here: #17177

slavapestov · 2018-06-27T20:55:10Z

Closing as discussed

Improve sil_combiner using subtyping information from CHA

4a6d5cd

rajbarik force-pushed the raj-cp branch from 0770b7f to 4a6d5cd Compare June 1, 2018 20:26

Support for structs and enums; moved CHA out of CTA;

ec5b670

change the test case

1d65242

slavapestov requested changes Jun 8, 2018

View reviewed changes

slavapestov self-assigned this Jun 8, 2018

gottesmm reviewed Jun 9, 2018

View reviewed changes

rajbarik mentioned this pull request Jun 13, 2018

ProtocolConformanceAnalysis #17177

Merged

rajbarik mentioned this pull request Jun 20, 2018

Concrete type propagation using ProtocolConformanceAnalysis #17373

Merged

slavapestov closed this Jun 27, 2018

swift-ci mentioned this pull request Jun 7, 2018

[SR-7891] Property requirement getter/setter not devirtualized #50426

Open

		@@ -0,0 +1,98 @@
		//===--- ConcreteTypeAnalysis.cpp - Protocol to Class inheritance ---------===//

		@@ -0,0 +1,139 @@
		// RUN: %target-swift-frontend %s -O -wmo -emit-sil -Xllvm -sil-disable-pass=DeadFunctionElimination \| %FileCheck %s

		internal protocol SomeProtocol : class {

		@@ -0,0 +1,70 @@
		#ifndef SWIFT_SILOPTIMIZER_ANALYSIS_CONCRETETYPE_H

Improve sil_combiner using subtyping information from CHA #16739

Improve sil_combiner using subtyping information from CHA #16739

Uh oh!

Conversation

rajbarik commented May 19, 2018 • edited by gottesmm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajbarik commented May 19, 2018

Uh oh!

slavapestov commented May 19, 2018

Uh oh!

gottesmm commented May 19, 2018

Uh oh!

rajbarik commented May 21, 2018

Uh oh!

gottesmm commented May 21, 2018

Uh oh!

rajbarik commented Jun 1, 2018

Uh oh!

rajbarik commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajbarik commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Jun 27, 2018

Uh oh!

Uh oh!

rajbarik commented May 19, 2018 •

edited by gottesmm

Loading

slavapestov commented Jun 8, 2018 •

edited

Loading

rajbarik commented Jun 13, 2018 •

edited

Loading