Opaque result types: dynamic replacement #24201

aschwaighofer · 2019-04-22T15:36:57Z

No description provided.

This is to support dynamic function replacement of functions with opaque result type. This approach requires that all state is thrown away (that could contain the old returned type for an opaque type) between replacements. rdar://48887938

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

Fixes rdar://problem/50001229.

aschwaighofer · 2019-04-22T15:37:05Z

@swift-ci Please test

swift-ci · 2019-04-22T16:38:14Z

Build failed
Swift Test Linux Platform
Git Sha - a0387ee

slavapestov

Sorry, I don’t know why some of my comments are not rendered properly.

slavapestov · 2019-04-22T16:32:38Z

lib/IRGen/Linking.cpp

@@ -576,6 +592,10 @@ SILLinkage LinkEntity::getLinkage(ForDefinition_t forDefinition) const {
  case Kind::ProtocolRequirementsBaseDescriptor:
  case Kind::MethodLookupFunction:
  case Kind::OpaqueTypeDescriptor:
+  case Kind::OpaqueTypeDescriptorAccessor:


Do these really need to be public?

Yed, they are what is getting replaced.

slavapestov · 2019-04-22T16:33:09Z

include/swift/IRGen/Linking.h

@@ -199,6 +199,10 @@ class LinkEntity {
    /// The descriptor for an opaque type.
    /// The pointer is an OpaqueTypeDecl*.
    OpaqueTypeDescriptor,
+    OpaqueTypeDescriptorAccessor,


These should have comments

slavapestov · 2019-04-22T17:01:30Z

@aschwaighofer It looks like when dynamic replacement is off, we directly reference the opaque type descriptor and call a runtime function to get the concrete metadata. Is this correct? Then why do we need to emit an accessor function when dynamic replacement is on? Can we not dynamically replace a global variable, perhaps storing a pointer inside an indirect global variable?

Do the new symbols get emitted even when dynamic replacement is off?

Here's an example showing you can have a 'some P' type that's in parameter position, or nested inside a structural return type:

protocol P {}

protocol Q {
  associatedtype T

  func f() -> T
}

struct S : Q {
  func f() -> some P { return ... }
}

struct UsesSomePInFunnyWay {
  func blah(_ x: S.T) -> Array<S.T> { return [x] }
}

Perhaps dynamic replacement can't support this yet, but it should degrade gracefully instead of crashing in a cast.

slavapestov · 2019-04-22T17:02:36Z

lib/IRGen/GenDecl.cpp

+  SmallVector<OpaqueTypeArchetypeType *, 8> newFuncTypes;
+  SmallVector<OpaqueTypeArchetypeType *, 8> origFuncTypes;
+  for (auto *newFunc : DynamicReplacements) {
+    if (!newFunc->getLoweredFunctionType()->hasOpaqueArchetype())


This should be a structural walk over the SILFunctionType using newFunc->getLoweredFunctionType().each()

slavapestov · 2019-04-22T17:04:02Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    auto argType = substConv.getSILArgumentType(argIdx);
+    if (argConv.isIndirectConvention() &&
+        argType.getASTType()->hasOpaqueArchetype() &&
+        !opd.get()->getType().getASTType()->hasOpaqueArchetype()) {


Maybe just check that the types differ? What if both have opaque types but they're different, because of various inlining and specialization passes?

slavapestov · 2019-04-22T17:04:16Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+      for (auto &inst : BB) {
+        auto *allocStack = dyn_cast<AllocStackInst>(&inst);
+        if (!allocStack ||
+            !allocStack->getElementType().is<OpaqueTypeArchetypeType>())


What if the element type contains an opaque type but its not at the top level?

slavapestov · 2019-04-22T17:04:28Z

lib/Sema/TypeCheckAttr.cpp

@@ -2093,6 +2093,94 @@ static Type getDynamicComparisonType(ValueDecl *value) {
  return interfaceType->removeArgumentLabels(numArgumentLabels);
 }

+static OpaqueTypeArchetypeType *getOpaqueResulType(const AbstractFunctionDecl *func) {


typo, should be "ResultType"

slavapestov · 2019-04-22T17:05:11Z

lib/Sema/TypeCheckAttr.cpp

+}
+
+static bool
+matchFunctionWithOpaqueResultType(const AbstractFunctionDecl *replacement,


Maybe instead this could be a flag for TypeBase::matches(), TypeMatchFlags::CompareOpaqueTypes or something? It really doesn't need to handle parameters and results differently, the only difference is when it finds an opaque type, instead of checking for equality it checks if they have the same constraints.

We'll need that flag when we allow non-final class methods to have opaque result types for instance...

aschwaighofer · 2019-04-22T17:25:07Z

@slavapestov

It looks like when dynamic replacement is off, we directly reference the opaque type descriptor and call a runtime function to get the concrete metadata. Is this correct? Then why do we need to emit an accessor function when dynamic replacement is on? Can we not dynamically replace a global variable, perhaps storing a pointer inside an indirect global variable?

I reuse the mechanism of dynamically replacing a function which is already present. I don't want to invent a new mechanism/runtime support for a feature that will be supplanted by something else.

Do the new symbols get emitted even when dynamic replacement is off?

No it should not. (see emitOpaqueTypeDescriptorAccessor) but there is a bug that I missed the same check (getAddressOfOpaqueTypeDescriptor) that I have to fix.

slavapestov · 2019-04-22T17:35:39Z

@aschwaighofer Ok, if you're going to replace this soon anyway then the current approach is fine. Do you have a radar or JIRA for the followup change? I'm curious to learn more about it.

Do you mind still tidying up the walks over the type to fish out any opaque types it contains, though, as well as the function type replacement matching in TypeCheckAttr.cpp?

aschwaighofer · 2019-04-22T17:39:25Z

Yes I will look at fixing nested opaque types.

aschwaighofer · 2019-04-22T21:06:52Z

@swift-ci Please test

swift-ci · 2019-04-22T21:09:03Z

Build failed
Swift Test Linux Platform
Git Sha - a0387ee

swift-ci · 2019-04-22T21:10:52Z

Build failed
Swift Test OS X Platform
Git Sha - a0387ee

aschwaighofer · 2019-04-22T22:11:48Z

@swift-ci Please test

swift-ci · 2019-04-22T22:12:28Z

Build failed
Swift Test OS X Platform
Git Sha - efe7bef

swift-ci · 2019-04-22T22:13:57Z

Build failed
Swift Test Linux Platform
Git Sha - efe7bef

slavapestov · 2019-04-23T00:46:18Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    auto argConv = apply.getArgumentConvention(opd);
+    auto argIdx = apply.getCalleeArgIndex(opd);
+    auto argType = substConv.getSILArgumentType(argIdx);
+    if (argConv.isIndirectConvention() &&


Can an opaque archetype be loadable (hence the argument convention is direct) if its protocol is class-constrained? I think you want to handle that by emitting an unchecked_ref_cast

Yes. Fixed. Test case added.

slavapestov · 2019-04-23T00:46:58Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    bool foundOpaqueArchetype = false;
+    for (auto &BB : *getFunction()) {
+      for (auto &inst : BB) {
+        auto *allocStack = dyn_cast<AllocStackInst>(&inst);


Why not just look for any instruction containing an opaque type?

Good point.

slavapestov · 2019-04-23T00:47:32Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

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


Indentation

clang-format does that.

slavapestov · 2019-04-23T00:47:37Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

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


Indentation

clang-format does that.

slavapestov · 2019-04-23T00:47:50Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

+    ReplaceOpaqueTypesWithUnderlyingTypes replacer;
+    return SuperTy::remapSubstitutionMap(Subs).subst(
+        replacer, replacer,
+        SubstFlags::SubstituteOpaqueArchetypes | SubstFlags::AllowLoweredTypes);


This formatting is not consistent with the previous lines

clang-format does that.

slavapestov · 2019-04-23T00:49:42Z

test/SILOptimizer/specialize_opaque_type_archetypes.swift

+  return t
+}
+
+// CHECK-LABEL: sil @$s1A10testFooBaryyxAA1PRzlF : $@convention(thin) <T where T : P> (@in_guaranteed T) -> () {


Can you add a few more test cases?

concrete type is address-only

the protocol is class-constrained

the opaque type is in another module (so it can't be specialized)

assignment to a stored property whose type is opaque (use the typealias/associated type trick I showed you in the earlier comment). I believe this will crash right now because you have to insert a cast, just like you do for function arguments

slavapestov · 2019-04-23T00:51:28Z

lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp

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


If the opaque type is from a different module (and hence its concrete type is not known to us) this is going to do a lot of wasted work. Can you check for feasibility before cloning the entire function?

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

omochi · 2019-04-23T04:30:18Z

Why these two functionality are in one pull request?
In my understand, specialization of ORT is compile time optimization that replacement resilient representation to underlying concrete type to improve performance when caller and callee are in same module.
On the other hand, dynamic replacement is replacement function implementation in loading time which is very early timing with loading dynamic library in runtime.
Discussion is this. https://forums.swift.org/t/dynamic-method-replacement/16619

So compiler implementation of specialization is independent from dynamic replacement support for ORT.
Am I missing something?

This comment is not criticism about development process.
I am hobby compiler learner and just want to know about internal source code structure.

aschwaighofer · 2019-04-23T14:22:07Z

No good reason.

Look at all instructions whether we have to cast. Return/YieldInst must not change the returned type.

aschwaighofer · 2019-04-23T18:01:51Z

This does not yet have the fix for stores ...

aschwaighofer · 2019-04-23T19:27:35Z

I am going to remove the OpaqueArchetypeSpecializer from this PR for now.

aschwaighofer · 2019-04-23T19:28:33Z

@swift-ci Please test

swift-ci · 2019-04-23T19:29:05Z

Build failed
Swift Test OS X Platform
Git Sha - 612a87f

swift-ci · 2019-04-23T19:31:07Z

Build failed
Swift Test Linux Platform
Git Sha - 612a87f

aschwaighofer · 2019-04-23T22:13:07Z

I have moved the specializer to a separate PR here: #24224

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

@swift-ci Please test

omochi · 2019-04-24T04:24:08Z

Thanks to response.

slavapestov · 2019-04-25T13:06:58Z

Thanks for addressing the review feedback, LGTM!

aschwaighofer added 8 commits April 22, 2019 07:31

Add a pass to specialize opaque type archetypes.

7ccdd00

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

This test requires debug mode

56f3949

Adjust test for 32bit platforms

f1877e5

Remove code I added for a previous implementation

a9fe7f1

Fix tests

f0a3b8b

[IRGen/TBDGen] Minor fixes for opaque result types.

bd2bee7

Fixes rdar://problem/50001229.

Opaque result types: Dynamic replacement fixes for computed properties

a0387ee

slavapestov reviewed Apr 22, 2019

View reviewed changes

aschwaighofer added 6 commits April 22, 2019 11:11

Only use opaque type accessors when the function is dynamic

6358c8f

Add comments

7d0bcda

IRGen: Traverse function type to find opaque archetypes

6184bb6

Add AllowCompatibleOpaqueTypeArchetypes as a Type.match flag

1fbf5f1

OpaqueArchetypeSpecializer addressed review comments

31effa7

Attempt to fix test case on linux bot

efe7bef

IRGen: Cleanup through use of IRGenFunction

59ec94b

Disable test case on linux for now

612a87f

slavapestov reviewed Apr 23, 2019

View reviewed changes

Fixes for OpaqueArchetypeSpecializer

14089c1

Look at all instructions whether we have to cast. Return/YieldInst must not change the returned type.

Remove SpecializeOpaqueArchetypes for now

f55d68d

aschwaighofer changed the title ~~Opaque result types: dynamic replacement and specialization~~ Opaque result types: dynamic replacement Apr 23, 2019

aschwaighofer merged commit 43a7b44 into swiftlang:master Apr 24, 2019

Opaque result types: dynamic replacement #24201

Opaque result types: dynamic replacement #24201

Uh oh!

Conversation

aschwaighofer commented Apr 22, 2019

Uh oh!

aschwaighofer commented Apr 22, 2019

Uh oh!

swift-ci commented Apr 22, 2019

Uh oh!

slavapestov 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!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 22, 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

slavapestov commented Apr 22, 2019

Uh oh!

aschwaighofer commented Apr 22, 2019

Uh oh!

aschwaighofer commented Apr 22, 2019

Uh oh!

swift-ci commented Apr 22, 2019

Uh oh!

swift-ci commented Apr 22, 2019

Uh oh!

aschwaighofer commented Apr 22, 2019

Uh oh!

swift-ci commented Apr 22, 2019

Uh oh!

swift-ci commented Apr 22, 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!

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!

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

Uh oh!

slavapestov Apr 22, 2019 •

edited

Loading

aschwaighofer commented Apr 22, 2019 •

edited

Loading

omochi commented Apr 23, 2019 •

edited

Loading