Add a pass to specialize opaque type archetypes. #24224

aschwaighofer · 2019-04-23T22:09:41Z

Clones functions containing opaque type archetypes replacing the opaque
type by a concrete type.

rdar://46140751

aschwaighofer · 2019-04-23T22:11:50Z

This does not check for resilience.

Resilience is currently not respected by type replacement: https://github.com/apple/swift/blob/master/lib/AST/Type.cpp#L2459

aschwaighofer · 2019-04-23T22:12:49Z

@swift-ci Please test

slavapestov · 2019-04-23T22:21:53Z

Ah, so basically all opaque types are eliminated by this pass today?

aschwaighofer · 2019-04-23T22:48:19Z

Yes until we fix https://github.com/apple/swift/blob/master/lib/AST/Type.cpp#L2459

aschwaighofer · 2019-04-23T22:49:54Z

I don't know the right answer yet how to do resilience properly: Maybe don't specialize if the namingDeclaration is resilient.

aschwaighofer · 2019-04-23T22:52:34Z

@swift-ci Please test

swift-ci · 2019-04-23T22:54:35Z

Build failed
Swift Test Linux Platform
Git Sha - dbf7f64

swift-ci · 2019-04-23T23:44:15Z

Build failed
Swift Test OS X Platform
Git Sha - dbf7f64

slavapestov · 2019-04-24T00:22:20Z

I think we should just say the opaque type is resilient if the module it’s in is resilient.

Making it depend on the resilience of the underlying nominal seems like an unnecessary complication. Also the underlying nominal could be something like Array which is fragile, but you might be returning a ‘some Sequence’ still expecting it to be resilient.

Finally inlinable functions would be an exception because obviously if you can see the body of the function the opaque type is not resilient.

What do you think @jckarter?

slavapestov · 2019-04-24T00:25:17Z

We should also add a test to validation-test/Evolution which changes an opaque type resiliently, once this is implemented.

aschwaighofer · 2019-04-24T01:49:22Z

I meant we would call ‘’opaquedecl->getNamingDecl()’’ which returns either a abstractstoragedecl or a func decl. From there we would figure out if that decl is resilient or not in our context: here is where I am more fuzzy and would need to look at how our APIs work. But we could apply the rules you mentioned if the decl is in a different resilient module it is resilient except if the funcdecl is inline able. Or maybe I can get from the naming decl to the sil function and use SIL apis. I will look into this.

aschwaighofer · 2019-04-24T11:02:28Z

@swift-ci Please test linux

swift-ci · 2019-04-24T12:14:47Z

Build failed
Swift Test Linux Platform
Git Sha - 70f5fd0

aschwaighofer · 2019-04-24T14:47:04Z

@swift-ci Please test

swift-ci · 2019-04-24T14:49:17Z

Build failed
Swift Test Linux Platform
Git Sha - 70f5fd0

swift-ci · 2019-04-24T14:51:23Z

Build failed
Swift Test OS X Platform
Git Sha - 70f5fd0

aschwaighofer · 2019-04-24T18:57:21Z

@swift-ci Please test

swift-ci · 2019-04-24T18:59:14Z

Build failed
Swift Test OS X Platform
Git Sha - 3beb095

swift-ci · 2019-04-24T19:00:55Z

Build failed
Swift Test Linux Platform
Git Sha - 3beb095

aschwaighofer · 2019-04-24T22:02:08Z

@swift-ci Please test linux

aschwaighofer · 2019-04-25T14:37:31Z

@swift-ci Please smoke test

aschwaighofer · 2019-04-25T16:21:51Z

@swift-ci Please smoke test

jckarter · 2019-04-25T21:59:21Z

@slavapestov I had hoped to get some feedback from you on how best to deal with resilience. It looks like Arnold has the basic criteria implemented—the underlying type is resilient if the module is resilient, or if the body of the NamingDecl is inlinable. However, should we pass down a ResilienceExpansion so that we do the right thing when specializing in the bodies of methods that are themselves inlinable and things like that?

jckarter · 2019-04-25T22:00:15Z

For instance, if you had:

public func foo() -> some P { ... }

@inlinable public func bar() { _ = foo() }

you wouldn't necessarily want to expose foo()'s underlying return type to other modules that inline bar.

slavapestov · 2019-04-26T23:52:20Z

include/swift/AST/ProtocolConformanceRef.h

-  ProtocolConformanceRef substOpaqueTypesWithUnderlyingTypes(Type origType) const;
-
+  ProtocolConformanceRef substOpaqueTypesWithUnderlyingTypes(
+      Type origType, ModuleDecl *modulePerformingSubstitution) const;


Can you move this method, SubstitutionMap::substOpaqueTypesWithUnderlyingTypes() and ReplaceOpaqueTypesWithUnderlyingTypes to your SIL pass since that's the only place where they're used now?

slavapestov · 2019-04-26T23:53:05Z

include/swift/SIL/TypeSubstCloner.h

@@ -109,7 +109,11 @@ class TypeSubstCloner : public SILClonerWithScopes<ImplClass> {

      assert(Subs.empty() ||
             SubstCalleeSILType ==
-                 Callee->getType().substGenericArgs(AI.getModule(), Subs));


This assertion is a bit silly now, how about just removing it instead of teaching TypeSubstCloner about opaque types

slavapestov · 2019-04-26T23:53:42Z

lib/AST/Type.cpp

+  auto namingDecl = opaqueRoot->getDecl()->getNamingDecl();
+  auto module = namingDecl->getModuleContext();
+  if (module->isResilient() && module != modulePerformingSubstitution &&
+      !namingDecl->getAttrs().hasAttribute<InlinableAttr>()) {


Please us getResilienceExpansion() instead of directly checking for an inlinable attribute; you're missing a number of cases this way

slavapestov · 2019-04-26T23:54:48Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+
+   // Apply the opaque types substitution.
+    ReplaceOpaqueTypesWithUnderlyingTypes replacer(currentModule);
+    Sty = Ty.subst(Original.getModule(), SubsMap)


since SubsMap is just the forwarding substitutions I don't think you need the first call to subst() at all, it will do nothing

slavapestov · 2019-04-26T23:55:04Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    return SuperTy::remapConformance(type, conf)
+        .subst(type, replacer, replacer,
+               SubstFlags::SubstituteOpaqueArchetypes |
+                   SubstFlags::AllowLoweredTypes);


AllowLoweredTypes isn't needed here

slavapestov · 2019-04-26T23:55:06Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    return SuperTy::remapASTType(ty)
+        .subst(replacer, replacer,
+               SubstFlags::SubstituteOpaqueArchetypes |
+                   SubstFlags::AllowLoweredTypes)


AllowLoweredTypes isn't needed here

slavapestov · 2019-04-26T23:55:41Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+      ReplaceOpaqueTypesWithUnderlyingTypes replacer(currentModule);
+      return ty.subst(replacer, replacer,
+                      SubstFlags::SubstituteOpaqueArchetypes |
+                          SubstFlags::AllowLoweredTypes)


This is pretty heavy weight. How about walking the type and seeing if it contains an opaque type from the current module?

slavapestov · 2019-04-26T23:56:07Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+        if (foundOpaqueArchetype)
+          break;
+        auto *allocStack = dyn_cast<AllocStackInst>(&inst);
+        if (!allocStack || !opaqueArchetypeWouldChange(


I don't understand why AllocStackInsts are special. You should specialize a function even if it doesn't contain any AllocStackInsts

slavapestov · 2019-04-26T23:56:44Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+
+    if (foundOpaqueArchetype) {
+      SubstitutionMap subsMap = getFunction()->getForwardingSubstitutionMap();
+      OpaqueSpecializerCloner s(subsMap, *getFunction());


the subsMap should not be used at all, I think you can try removing it. It's just burning CPU cycles to substitute using forwarding substitutions. Maybe the opaque cloner should directly inherit from SILCloner and not TypeSubstCloner?

slavapestov · 2019-04-26T23:58:10Z

@jckarter @aschwaighofer Yeah, I think you should look at SILFunction::getResilienceExpansion(). If a function's resilience expansion is Minimal, you should not specialize any opaque types except those from other inlinable functions.

slavapestov · 2019-04-27T00:00:11Z

lib/AST/Type.cpp

@@ -2491,6 +2501,15 @@ ReplaceOpaqueTypesWithUnderlyingTypes::operator()(CanType maybeOpaqueType,

  auto archetype = archetypeAndRoot->first;
  auto opaqueRoot = archetypeAndRoot->second;
+
+  // Don't replace opaque types from resilient modules.


This bit of code is copy and pasted twice, please turn it into a method on ReplaceOpaqueTypesWithUnderlyingTypes. Also it should take the resilience expansion into account:

If the opaque type is from an inlinable function, replace opaque type with the underlying type.

If expansion is Maximal and we're in the same module, replace the opaque type with the underlying type.

If expansion is Minimal and we didn't hit the first case, do nothing.

slavapestov · 2019-04-27T00:00:55Z

lib/SIL/SILFunctionType.cpp

@@ -2465,7 +2476,7 @@ SILFunctionType::substGenericArgs(SILModule &silModule,
                                  LookupConformanceFn conformances) {
  if (!isPolymorphic()) return CanSILFunctionType(this);
  SILTypeSubstituter substituter(silModule, subs, conformances,
-                                 getGenericSignature());
+                                 getGenericSignature(), false);


Add a comment here, /*shouldSubstituteOpaqueTypes*/ false

aschwaighofer · 2019-04-29T17:13:17Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+  return shouldPerformSubstitution(opaque, context);
+}
+
+bool ReplaceOpaqueTypesWithUnderlyingTypes::shouldPerformSubstitution(


@slavapestov Can you check whether you agree with these rules:

if the function with the opaque result type is inline-able allow substitution

if the context function in which we perform substitution has maximal resilience expansion and the context function and the opaque type's function are in the same module allow substitution

if the opaque types's module is not resilient allow substitution

otherwise disallow substitution

aschwaighofer · 2019-05-01T15:16:21Z

@swift-ci Please test

swift-ci · 2019-05-01T16:13:50Z

Build failed
Swift Test OS X Platform
Git Sha - cfc34b16b6f5df19349d64374973d0327c22bda7

swift-ci · 2019-05-01T16:14:50Z

Build failed
Swift Test Linux Platform
Git Sha - cfc34b16b6f5df19349d64374973d0327c22bda7

Clones functions containing opaque type archetypes replacing the opaque type by a concrete type. rdar://46140751

We might have to replace opaque archetypes to satisfy equality of types

…aque result types

* Opaque types are abi compatible with their substituted types * Insert casts in more places * Respect no optimization attribute

aschwaighofer · 2019-05-01T16:32:18Z

@swift-ci Please test

swift-ci · 2019-05-01T16:32:35Z

Build failed
Swift Test Linux Platform
Git Sha - cfc34b16b6f5df19349d64374973d0327c22bda7

swift-ci · 2019-05-01T16:32:40Z

Build failed
Swift Test OS X Platform
Git Sha - cfc34b16b6f5df19349d64374973d0327c22bda7

slavapestov · 2019-05-02T12:08:41Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

@@ -0,0 +1,466 @@
+#define DEBUG_TYPE "opaque-archetype-specializer"


This needs a copyright notice, etc

slavapestov · 2019-05-02T12:08:56Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+
+static Type substOpaqueTypesWithUnderlyingTypes(
+    Type ty, SILFunction *context) {
+  ReplaceOpaqueTypesWithUnderlyingTypes replacer(context);


Please move ReplaceOpaqueTypesWithUnderlyingTypes into this file

It is now also used by lib/SIL/SILFunctionType.cpp because substituted and unsubstituted are ABI compatible.

slavapestov · 2019-05-02T12:09:39Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    if (!getBuilder().hasOwnership()) {
+      switch (Inst->getOwnershipQualifier()) {
+      case StoreOwnershipQualifier::Assign: {
+        auto *li = getBuilder().createLoad(getOpLocation(Inst->getLoc()), dst,


Why do you lower assigns to load/store here instead of letting them be?

Same code as SILCloner.

Why does it need to appear here?

slavapestov · 2019-05-02T12:10:14Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+          auto cast = getBuilder().createUncheckedAddrCast(apply.getLoc(),
+                                                           opd.get(), argType);
+          opd.set(cast);
+        } else if (argType.is<SILFunctionType>()) {


What if its an optional of a function type, or a tuple type, etc? You need to recursively transform types instead

Good point! I will go through memory for now in those cases.

compnerd · 2019-05-02T18:18:27Z

This broke the windows build! https://dev.azure.com/compnerd/windows-swift/_build/results?buildId=1963

aschwaighofer · 2019-05-02T18:49:02Z

I will fix.

aschwaighofer · 2019-05-02T19:15:43Z

@compnerd I hope that is the circular dependency issue I introduced. Should be fixed by #24443

aschwaighofer mentioned this pull request Apr 23, 2019

Opaque result types: dynamic replacement #24201

Merged

slavapestov reviewed Apr 26, 2019

View reviewed changes

slavapestov reviewed Apr 27, 2019

View reviewed changes

aschwaighofer commented Apr 29, 2019

View reviewed changes

aschwaighofer added 9 commits May 1, 2019 09:31

Add a pass to specialize opaque type archetypes.

fbf0903

Clones functions containing opaque type archetypes replacing the opaque type by a concrete type. rdar://46140751

Projections should keep the type of the underlying type

9f83f4b

Fix assert in type substitution cloner

881d9ad

We might have to replace opaque archetypes to satisfy equality of types

Respect resilience when specializing opaque type archetypes

a793dfb

Look through resilient inlinable functions when specializing their op…

cc2a7c1

…aque result types

Add test cases for computed properties

ef78844

Address review feedback

768d1c5

More fixes for opaque types specializer

cec1a52

* Opaque types are abi compatible with their substituted types * Insert casts in more places * Respect no optimization attribute

Fix IRGen test lazy_opaque_result_type.swift

0e313c7

aschwaighofer force-pushed the specialize_opaque_result_types branch from cfc34b1 to 0e313c7 Compare May 1, 2019 16:31

aschwaighofer merged commit aa376a4 into swiftlang:master May 2, 2019

slavapestov reviewed May 2, 2019

View reviewed changes

davezarzycki mentioned this pull request May 2, 2019

[CMake] Workaround circular linkage dependency #24439

Closed

		@@ -0,0 +1,466 @@
		#define DEBUG_TYPE "opaque-archetype-specializer"

Add a pass to specialize opaque type archetypes. #24224

Add a pass to specialize opaque type archetypes. #24224

Uh oh!

Conversation

aschwaighofer commented Apr 23, 2019

Uh oh!

aschwaighofer commented Apr 23, 2019

Uh oh!

aschwaighofer commented Apr 23, 2019

Uh oh!

slavapestov commented Apr 23, 2019

Uh oh!

aschwaighofer commented Apr 23, 2019

Uh oh!

aschwaighofer commented Apr 23, 2019

Uh oh!

aschwaighofer commented Apr 23, 2019

Uh oh!

swift-ci commented Apr 23, 2019

Uh oh!

swift-ci commented Apr 23, 2019

Uh oh!

slavapestov commented Apr 24, 2019

Uh oh!

slavapestov commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 24, 2019

Uh oh!

swift-ci commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 24, 2019

Uh oh!

swift-ci commented Apr 24, 2019

Uh oh!

swift-ci commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 24, 2019

Uh oh!

swift-ci commented Apr 24, 2019

Uh oh!

swift-ci commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 24, 2019

Uh oh!

aschwaighofer commented Apr 25, 2019

Uh oh!

aschwaighofer commented Apr 25, 2019

Uh oh!

jckarter commented Apr 25, 2019

Uh oh!

jckarter commented Apr 25, 2019

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 Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschwaighofer Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

aschwaighofer Apr 29, 2019 •

edited

Loading