ExistentialSpecializer Pass (without SILCombine/ConcreteType Propagation) #17366

rajbarik · 2018-06-20T18:23:19Z

ExistentialSpecializer transforms functions with existential arguments to generic parameter types.
That is, it transforms from

func foo(a:P) -> Int { // P is an existential that is a protocol or a protocol composition type
 return a.bar()
}

to

@inline(always) func foo(a: P) -> Int {
  let _a = a open as T
  return _wrap_inc(_a)
}
func _foo<T : P>(_a:P) -> Int{
 let a: P = _a
 return a.bar()
}

With subsequent passes such as Inlining, Generic Specialization and SILCombine, the concrete type of P can be determined, which most likely will devirtualize a.bar() method call.

This is part of the large PR #13991
test cases will be added after the final code for SILCombine/ConcreteType Propagation lands.

rajbarik · 2018-06-20T18:24:43Z

@slavapestov Breaking the large PR from #13991 to this one as Part-1. Part-2 will be the changes to SILCombiner.

rajbarik · 2018-07-10T22:57:19Z

@slavapestov @atrick This is the transformation pass for existential specializer. Added a new test case and cleaned up the code as suggested by Slava.

slavapestov · 2018-06-27T21:10:07Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+} // namespace
+
+/// Find concrete type from init_existential refs/addrs.
+static bool findConcreteTypeFromIE(SILValue Arg, CanType &ConcreteType) {


Rename to findConcreteTypeFromInitExistential?

slavapestov · 2018-06-27T21:11:30Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+    auto ArgType = Arg->getType();
+    auto SwiftArgType = ArgType.getASTType();
+    if (!ArgType || !SwiftArgType || !(ArgType.isExistentialType()) ||
+        ArgType.isAnyObject() || SwiftArgType->isAny())


Please figure out why this doesn't work

This will cause code blowup due to specialization -- I will add a comment. We do not want to do this for now.

slavapestov · 2018-06-27T21:11:54Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+    auto Arg = Args[Idx];
+    auto ArgType = Arg->getType();
+    auto SwiftArgType = ArgType.getASTType();
+    if (!ArgType || !SwiftArgType || !(ArgType.isExistentialType()) ||


These will never be null

slavapestov · 2018-06-27T21:12:52Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+
+    /// Find concrete type.
+    CanType ConcreteType;
+    int ApplyArgIdx = PAI ? Idx - Num + minPartialAppliedArgs : Idx;


Maybe don't bother with PartialApplyInst

slavapestov · 2018-06-27T21:13:20Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+    return false;
+
+  /// External function definitions.
+  if ((!Callee->isDefinition()) || Callee->isExternalDeclaration())


isExternalDeclaration is checking the same thing as isDefinition

slavapestov · 2018-06-27T21:30:45Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  SILType LoweredType = NewF->getLoweredType();
+  SILFunctionConventions Conv(SubstCalleeType, M);
+  SILType ResultType = Conv.getSILResultType();
+  auto FunctionTy = LoweredType.castTo<SILFunctionType>();


Unnecessary cast

slavapestov · 2018-06-27T21:30:57Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+        NormalBlock->createPHIArgument(ResultType, ValueOwnershipKind::Owned);
+    SILBasicBlock *ErrorBlock = Thunk->createBasicBlock();
+    SILType Error =
+        SILType::getPrimitiveObjectType(FunctionTy->getErrorResult().getType());


getSILType()

slavapestov · 2018-06-27T21:31:32Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -238,6 +238,9 @@ void addSSAPasses(SILPassPipelinePlan &P, OptimizationLevelKind OpLevel) {
  // Promote stack allocations to values.
  P.addMem2Reg();

+  // Run the existential pecializer Pass.


Typo "specializer"

slavapestov · 2018-07-11T21:27:54Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+
+  /// Return the concrete type from init_existential returned from
+  /// findInitExistential.
+  if (auto *IER = dyn_cast<InitExistentialRefInst>(InitExistential)) {


Use findConcreteTypeFromInitExistential() here?

slavapestov · 2018-07-11T21:28:36Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+    auto Arg = Args[Idx];
+    auto ArgType = Arg->getType();
+    auto SwiftArgType = ArgType.getASTType();
+    if (!ArgType || !SwiftArgType || !(ArgType.isExistentialType()) ||


Unnecessary parentheses around ArgType.isExistentialType() check

rajbarik · 2018-07-11T22:02:24Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  /// Then we splice the body of F to NewF after the first basic block.
+  auto It = NewF->begin();
+  It++;
+  NewF->getBlocks().splice(It, F->getBlocks());


Instead of splice: use a TypeSubstCloner with a Builder and SubstitutionMap and then call the API visitSILFunction(original function F);

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

rajbarik · 2018-07-11T22:09:04Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+
+  unsigned int OrigDepth = 0;
+  if (F->getLoweredFunctionType()->isPolymorphic()) {
+    OrigDepth = OrigCalleeGenericSig->getGenericParams().back()->getDepth();


Make this OrigDepth = OrigCalleeGenericSig->getGenericParams().back()->getDepth()+1;
and then remove the check for polymorphic on line 457 to F->getLoweredFunctionType()->isPolymorphic() and then the 458 is now < not <=

slavapestov · 2018-07-11T21:43:38Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+
+  unsigned int OrigDepth = 0;
+  if (F->getLoweredFunctionType()->isPolymorphic()) {
+    OrigDepth = OrigCalleeGenericSig->getGenericParams().back()->getDepth();


slavapestov · 2018-07-11T21:43:47Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+        GenericTypeParamType *GP = nullptr;
+        if ((GP = dyn_cast<GenericTypeParamType>(type))) {
+          if (F->getLoweredFunctionType()->isPolymorphic() &&
+              GP->getDepth() <= OrigDepth) {


Remove the isPolymorphic check and use < instead of <=

slavapestov · 2018-07-11T21:44:10Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+          if (F->getLoweredFunctionType()->isPolymorphic() &&
+              GP->getDepth() <= OrigDepth) {
+            return QuerySubstitutionMap{OrigSubMap}(
+                cast<SubstitutableType>(GP->getCanonicalType()));


Type(GP).subst(OrigSubMap)

slavapestov · 2018-07-11T21:44:39Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+      CalleeGenericSig,
+      [&](SubstitutableType *type) -> Type {
+        GenericTypeParamType *GP = nullptr;
+        if ((GP = dyn_cast<GenericTypeParamType>(type))) {


if (auto *GP = dyn_cast<GenericTypeParamType>(type))

rajbarik · 2018-07-27T20:57:28Z

@slavapestov Could you also review this one? All your comments are taken care of, except generic methods.

rajbarik · 2018-08-30T00:36:33Z

@slavapestov can you check this now?

rajbarik · 2018-09-07T18:36:04Z

@slavapestov should be ready now?

slavapestov · 2018-09-10T06:00:25Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+    }
+
+    /// Determine attributes of the existential addr arguments such as
+    /// destroy_use, immutable_access. We should probably also disable InOut


What does "probably" mean here? Either they should be disabled if they don't work, or they should be enabled

slavapestov · 2018-09-10T06:00:54Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+
+    auto ExistentialRepr =
+        Arg->getType().getPreferredExistentialRepresentation(F->getModule());
+    if (!(ExistentialRepr == ExistentialRepresentation::Opaque ||


I would find if (ExistentialRepr != ExistentialRepresentation::Opaque && ExistentialRepr != ExistentialRepresentation::Class more readable

slavapestov · 2018-09-10T06:01:25Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+bool ExistentialSpecializer::canSpecializeCalleeFunction(ApplySite &Apply) {
+
+  /// Do not handle partial applies.
+  if (auto *PAI = dyn_cast<PartialApplyInst>(Apply.getInstruction())) {


You can use isa instead since you don't care about the value

slavapestov · 2018-09-10T06:01:59Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+  if (!Callee->isDefinition())
+    return false;
+
+  /// Ignore functions with indirect results.


Next PR will lift this!

slavapestov · 2018-09-10T06:02:03Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp

+  if (Callee->getConventions().hasIndirectSILResults())
+    return false;
+
+  /// Ignore error returning functions.


Next PR will life this.

slavapestov · 2018-09-10T06:11:49Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  /// Now add the yields.
+  llvm::SmallVector<SILYieldInfo, 8> InterfaceYields;
+  for (SILYieldInfo InterfaceYield : FTy->getYields()) {
+    InterfaceYields.push_back(InterfaceYield);


Don't you need to map this type out of context too?

slavapestov · 2018-09-10T06:12:21Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+      auto SwiftType = ArgDesc.Arg->getType().getASTType();
+      auto OpenedType =
+          SwiftType->openAnyExistentialType(Opened)->getCanonicalType();
+      auto OpenedSILType = NewF->getModule().Types.getLoweredType(OpenedType);


Maybe just F.getLoweredType(OpenedType)

This does not work!

There is no API in F that allows you to take a Type. Its an empty one.

That is F.getLoweredType does not take an argument.

slavapestov · 2018-09-10T06:12:42Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+        break;
+      }
+      };
+      Generic2OpenedTypeMap.insert(


Assert that the insert succeeded

slavapestov · 2018-09-10T06:13:05Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  }
+}
+
+/// Strategy to specialize existentail arguments:


Typo: "existential"

slavapestov · 2018-09-10T06:13:44Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+                           [&](SubstitutableType *type) -> Type {
+                             return NewFGenericEnv->mapTypeIntoContext(type);
+                           },
+                           LookUpConformanceInSignature(*NewFGenericSig));


Please just use LookUpConformanceInModule instead. LookUpConformanceInSignature is going away

rajbarik · 2018-09-17T23:00:23Z

@slavapestov please have a look.

slavapestov · 2018-09-21T21:55:52Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+    } else {
+      auto Ty = param.getType();
+      InterfaceParams.push_back(
+          SILParameterInfo(!Ty->hasTypeParameter()


if hasTypeParameter() returns false, mapTypeOutOfContext() does nothing. Perhaps remove this whole ?: operation and just use 'Ty'?

Unfortunately, mapTypeOutOfContext() in GenericEnvironment puts an assertion like this:
"assert(!hasTypeParameter() && "already have an interface type");"
So, I can not find an easy way to go around it.

If the type has type parameters it's already been "mapped out of context". You don't have to do anything here.

slavapestov

I think if you just fix the last mapTypeOutOfContext() nitpick and use SmallVector instead of SmallDenseMap<int, ...> (or explain why you can't) I will merge this.

slavapestov · 2018-09-25T04:31:13Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+      /// Arguments that are not rewritten.
+      auto Ty = params[ArgDesc.Index].getType();
+      auto MappedTy = ArgDesc.Arg->getType();
+      if (Ty->hasTypeParameter()) {


You don't need the 'if'. It should either be fine to map it in unconditionally, or if that always fails, just don't map it in at all.

slavapestov · 2018-09-25T04:32:33Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+                                                 param.getConvention()));
+    } else {
+      auto Ty = param.getType();
+      InterfaceParams.push_back(


You can replace this whole thing with InterfaceParams.push_back(param). You don't need to do anything with Ty.

slavapestov · 2018-09-25T04:32:45Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  llvm::SmallVector<SILResultInfo, 8> InterfaceResults;
+  for (auto &result : FTy->getResults()) {
+    auto Ty = result.getType();
+    InterfaceResults.push_back(result.getWithType(


You can replace this whole thing with InterfaceResults.push_back(result). You don't need to do anything with Ty.

Or why bother with InterfaceResults at all? You can remove that SmallVector altogether and use FTy->getResults() when you build the new function type instead. The only thing that's changing here is the parameter types, everything else comes directly from the old function type and doesn't need any remapping.

slavapestov · 2018-09-25T04:33:10Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  if (FTy->hasErrorResult()) {
+    auto errorResult = FTy->getErrorResult();
+    auto Ty = errorResult.getType();
+    InterfaceErrorResult = SILResultInfo(


This is just InterfaceErrorResult = errorResult.

slavapestov · 2018-09-25T04:33:21Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  llvm::SmallVector<SILYieldInfo, 8> InterfaceYields;
+  for (SILYieldInfo yield : FTy->getYields()) {
+    auto Ty = yield.getType();
+    InterfaceYields.push_back(yield.getWithType(


You can replace this whole thing with InterfaceYields.push_back(yield). You don't need to do anything with Ty.

slavapestov · 2018-09-25T04:34:26Z

@rajbarik What do you think of using a SmallVector instead of SmallDenseMap<int, ...>?

rajbarik · 2018-09-25T20:05:00Z

@rajbarik What do you think of using a SmallVector instead of SmallDenseMap<int, ...>?

An ExistentialTransformArgumentDescriptor is only built for an existential argument, not other arguments. So,SmallVector is not the right option? Am I missing anything?

slavapestov · 2018-09-25T21:12:13Z

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialTransform.cpp

+  /// Now add the errors.
+  Optional<SILResultInfo> InterfaceErrorResult;
+  if (FTy->hasErrorResult()) {
+    InterfaceErrorResult = FTy->getErrorResult();


In a follow-up PR, please remove this whole block. Instead, just pass FTy->getErrorResult() to SILFunctionType::get() below.

slavapestov · 2018-09-25T21:12:34Z

@swift-ci Please test

slavapestov · 2018-09-25T21:12:48Z

@swift-ci Please test source compatibility

slavapestov · 2018-09-25T21:13:46Z

@rajbarik I think this is good to merge now! Thank you for all your hard work. I'm going to merge it if the tests pass.

Next, can you make a single commit which enables it by default, and we'll run the test suite and benchmarks to see if there are any regressions?

slavapestov · 2018-09-25T21:13:55Z

@swift-ci Please test

slavapestov · 2018-09-25T21:14:00Z

@swift-ci Please test source compatibility

rajbarik · 2018-09-25T21:23:28Z

To enable this transformation by default, please change the following line in ExistentialSpecializer.cpp

if (!F->shouldOptimize() || !F->getModule().getOptions().ExistentialSpecializer)

to

if (!F->shouldOptimize())

slavapestov · 2018-09-25T21:26:24Z

@rajbarik Please make a PR that does changes the default flag value in SILOptions. Then we'll run PR testing on it.

slavapestov · 2018-09-25T21:29:40Z

@swift-ci Please test

slavapestov · 2018-09-25T21:29:46Z

@swift-ci Please test source compatibility

slavapestov · 2018-09-25T21:29:56Z

@swift-ci Please test

slavapestov · 2018-09-25T21:30:01Z

@swift-ci Please test

…ion)

slavapestov · 2018-09-25T22:16:31Z

@swift-ci Please test

swift-ci · 2018-09-25T22:18:19Z

Build failed
Swift Test Linux Platform
Git Sha - cabaf2fb41ca2aef104e26bef763c4121a548b6f

slavapestov · 2018-09-25T22:18:39Z

@swift-ci Please test

slavapestov self-assigned this Jun 27, 2018

rajbarik force-pushed the raj-existential2generic branch from aec0db1 to 23c6dd3 Compare July 10, 2018 22:52

rajbarik force-pushed the raj-existential2generic branch from 23c6dd3 to a8a1fab Compare July 11, 2018 21:26

slavapestov reviewed Jul 11, 2018

View reviewed changes

rajbarik commented Jul 11, 2018

View reviewed changes

lib/SILOptimizer/FunctionSignatureTransforms/ExistentialSpecializer.cpp Show resolved Hide resolved

rajbarik commented Jul 11, 2018

View reviewed changes

slavapestov reviewed Jul 11, 2018

View reviewed changes

rajbarik force-pushed the raj-existential2generic branch from a8a1fab to cc60047 Compare July 27, 2018 20:56

rajbarik mentioned this pull request Aug 3, 2018

Concrete type propagation using ProtocolConformanceAnalysis #17373

Merged

rajbarik force-pushed the raj-existential2generic branch from cc60047 to 71d8c70 Compare August 30, 2018 00:34

rajbarik force-pushed the raj-existential2generic branch 5 times, most recently from 2642368 to 700d019 Compare September 5, 2018 19:15

rajbarik force-pushed the raj-existential2generic branch from 700d019 to 9592802 Compare September 6, 2018 19:42

slavapestov reviewed Sep 10, 2018

View reviewed changes

rajbarik force-pushed the raj-existential2generic branch from 9592802 to b809ac7 Compare September 17, 2018 18:28

slavapestov reviewed Sep 21, 2018

View reviewed changes

slavapestov approved these changes Sep 21, 2018

View reviewed changes

slavapestov reviewed Sep 25, 2018

View reviewed changes

rajbarik force-pushed the raj-existential2generic branch from b809ac7 to 23a6ef7 Compare September 25, 2018 20:08

slavapestov approved these changes Sep 25, 2018

View reviewed changes

rajbarik force-pushed the raj-existential2generic branch from 23a6ef7 to cabaf2f Compare September 25, 2018 21:28

ExistentialSpecializer Pass (without SILCombine/ConcreteType Propagat…

89d31be

…ion)

rajbarik force-pushed the raj-existential2generic branch from cabaf2f to 89d31be Compare September 25, 2018 21:52

rajbarik merged commit a410312 into swiftlang:master Sep 26, 2018

ExistentialSpecializer Pass (without SILCombine/ConcreteType Propagation) #17366

ExistentialSpecializer Pass (without SILCombine/ConcreteType Propagation) #17366

Uh oh!

Conversation

rajbarik commented Jun 20, 2018

Uh oh!

rajbarik commented Jun 20, 2018

Uh oh!

rajbarik commented Jul 10, 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!

Choose a reason for hiding this comment

Uh oh!

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!

rajbarik commented Jul 27, 2018

Uh oh!

rajbarik commented Aug 30, 2018

Uh oh!

rajbarik commented Sep 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment