Emit and call thunks for back deployed functions #41416

tshortli · 2022-02-16T23:10:48Z

Emit a thunk that wraps the call to a function with the @_backDeploy attribute. The thunk calls the original function if it is available at runtime and otherwise falls back to calling a copy that is emitted into the client:

  func X_thunk(...) async throws -> ... {
    if #available(...) {
      return try await X(...)
    } else {
      return try await X_clientCopy(...)
    }
  }

Resolves rdar://89122645

… The thunk calls the original function if it is available at runtime and otherwise falls back to calling a copy that is emitted into the client (not yet implemented).

tshortli · 2022-02-18T03:24:58Z

@swift-ci please test

slavapestov

Great work! I think it's mostly complete, I just have a few suggestions.

slavapestov · 2022-02-18T21:30:40Z

include/swift/AST/Decl.h

@@ -6268,6 +6268,9 @@ class AbstractFunctionDecl : public GenericContext, public ValueDecl {
  /// Returns 'true' if the function is distributed.
  bool isDistributed() const;

+  /// Returns 'true' if the function can be back deployed.


This comment doesn't really add any value in my opinion -- either it should be removed, or clarified with an explanation of the actual behavior.

slavapestov · 2022-02-18T21:33:11Z

lib/SIL/IR/SILDeclRef.cpp

-
+
+    // Back deployment support functions are not in the vtable.
+    if (backDeploymentKind != SILDeclRef::BackDeploymentKind::None)


Could this be instead enforced with a restriction preventing @_backDeploy from being applied to non-final methods?

Yes, I suppose that the eventual type checking restrictions that will be in place should prevent this from ever happening. Would it make sense to assert even then, though? One thing I don't have a good feel for yet is how much to count on type checking and how much to check its work later :)

Yeah, if something in SILGen is impossible and should have been caught by the type checker, asserting is the right approach. Trust but verify :)

So it turns out that this is called unconditionally and the early return is needed.

So it turns out that this is called unconditionally and the early return is needed.

Ah, this needs more thought then. When it is being called? I see that we call this for final methods that override something else. So the question is, is this supported:

class Base { func foo() {} } class Derived { @_backDeploy final override func foo() {} }

If so, this should do something more useful since back-deployed methods can appear in the vtable, they just cannot be overriden further.

If not, you might want to prohibit non-final and override methods from being @_backDeploy.

Sorry, I goofed here and put the assert before of the if (auto overridden = getOverridden()) branch which caused it to fail. It's fine to assert inside the branch so I've done that.

slavapestov · 2022-02-18T21:34:07Z

lib/SILGen/SILGenApply.cpp

@@ -1030,6 +1030,11 @@ class SILGenApply : public Lowering::ExprVisitor<SILGenApply> {
  }

  void processClassMethod(DeclRefExpr *e, AbstractFunctionDecl *afd) {
+    // FIXME(backDeploy): Investigate support for back deployed class method


Should be fine for final methods

That makes sense. I'm planning to come back through with a PR specifically aimed at making sure class methods with static dispatch are supported and that the attribute is rejected on methods with dynamic dispatch.

slavapestov · 2022-02-18T21:36:40Z

test/SILGen/back_deploy_attribute.swift

-  // CHECK: %4 = function_ref @$s21back_deploy_attribute14TopLevelStructV0a8DeployedF4FuncyyF : $@convention(method) (TopLevelStruct) -> ()
-  // CHECK: %5 = apply %4(%0) : $@convention(method) (TopLevelStruct) -> ()
+  // CHECK: [[FNREF:%.*]] = function_ref @$s21back_deploy_attribute14TopLevelStructV0a8DeployedF4FuncyyFTwb : $@convention(method) (TopLevelStruct) -> ()
+  // CHECK: [[FNRES:%.*]] = apply [[FNREF]]([[STRUCT_ARG]]) : $@convention(method) (TopLevelStruct) -> ()
  s.backDeployedStructFunc()
 }


Suggestions for more tests:

Complex parameter lists (indirectly-passed values like existentials, inout parameters, tuples)

Complex returns (indirectly-returned values like existentials, tuples)

Generic functions

Async and throws functions

Anything with a non-trivial body

Also please add tests in test/attr/ or test/Sema/ to ensure that inlinable restrictions are enforced in the body of a @_backDeploy just like they are for @_aeic.

Good point, I should cover many more types of functions. One thing I wonder about is the scalability of testing all those different function types as SILGen tests. Do you think it's important that we verify the SIL for each of those configurations, or would it be satisfactory to have a validation test that calls many different types of back deployed functions and verifies their behaviors? I was planning on writing the latter kind of test in upcoming PRs after a bit more of the type checking is in place.

I do have an existing test in /test/attr verifying the type checking diagnostics for this attribute. However, much of the type checking is still coming in later PRs. I wanted to get this working end to end to make sure I understood how it would work before adding all the polish.

I don't think it makes sense to cover every possible case, I was just thinking of the high-value ones that might identify bugs in how you're forwarding arguments and results.

Sounds good, I'll cover what you listed.

Testing generics revealed that the thunk is not quite forwarding arguments perfectly for all functions correctly yet so I'm continuing to investigate that.

I now have tests covering back deployment of the following function archetypes:

A trivial top level function with no arguments or results.

A top level function that takes an argument and returns a value.

A trivial top level function that throws.

A trivial top level function on a struct.

A generic function that takes and returns a T.

I also experimented with async functions, functions taking inout params, and functions taking/returning an existential but did not end up writing any SILGen test for those specifically because they didn't appear to me to exercise distinct logic in my SILGen code. I think there may be diminishing returns on more SILGen tests. However, I do intend to write executable validation tests that exercise a wider variety of function archetypes in an upcoming PR.

slavapestov · 2022-02-18T21:37:28Z

include/swift/SIL/SILDeclRef.h

@@ -162,6 +162,17 @@ struct SILDeclRef {
    AsyncEntryPoint,
  };

+  /// Represents the variants of a back deployable function.
+  enum class BackDeploymentKind : unsigned {
+    /// The original function.


This is the one that is part of the ABI, right?

That's right, I'll clarify that.

Updated comments

slavapestov · 2022-02-18T21:37:37Z

include/swift/SIL/SILDeclRef.h

+    /// The original function.
+    None,
+    /// The thunk variant of a function that calls either the original function
+    /// or the fallback variant if the original is unavailable.


Mention this one is @_aeic / PublicNonABI

Updated comments

slavapestov · 2022-02-18T21:37:42Z

include/swift/SIL/SILDeclRef.h

+    /// The thunk variant of a function that calls either the original function
+    /// or the fallback variant if the original is unavailable.
+    Thunk,
+    /// The fallback variant of the function.


Mention this one is @_aeic / PublicNonABI

Updated comments

slavapestov · 2022-02-18T21:41:05Z

lib/Demangling/NodePrinter.cpp

@@ -1414,6 +1416,7 @@ NodePointer NodePrinter::print(NodePointer Node, unsigned depth,
  case Node::Kind::CFunctionPointer:
  case Node::Kind::ObjCBlock:
  case Node::Kind::EscapingObjCBlock:
+  case Node::Kind::BackDeploymentFallback:


Mangling output should differentiate the fallback from the original function I think

Makes sense, I'll figure out how to accomplish that.

Fixed and added tests for the mangling classification. This revealed a number of issues with my existing mangling implementation which are now fixed.

…pdate the back deployment thunk mangling to use the same prefix.

tshortli · 2022-02-19T00:34:16Z

Great work! I think it's mostly complete, I just have a few suggestions.

Thanks for the review. I have addressed all the feedback except for adding more SILGen tests, which I'll pick up Monday.

…l it when the original function is not available at run time.

tshortli · 2022-02-24T02:07:40Z

@swift-ci please test

nkcsgexi · 2022-02-26T03:16:16Z

🎉

tshortli force-pushed the back-deploy-thunk branch from 3c576fc to 7895516 Compare February 17, 2022 01:46

tshortli added 3 commits February 17, 2022 11:28

SIL: Add flag to SILDeclRef for back deployment thunks.

2120438

Mangling: Use 'Tw' to mangle back deployment thunks.

ff5abc4

SILGen: Emit a thunk that wraps the call to a back deployed function.…

97e8e31

… The thunk calls the original function if it is available at runtime and otherwise falls back to calling a copy that is emitted into the client (not yet implemented).

tshortli force-pushed the back-deploy-thunk branch 2 times, most recently from 9c4b5a7 to af76a8d Compare February 18, 2022 03:20

tshortli marked this pull request as ready for review February 18, 2022 03:25

tshortli requested review from xymus, slavapestov and nkcsgexi February 18, 2022 03:25

slavapestov reviewed Feb 18, 2022

View reviewed changes

tshortli added 2 commits February 18, 2022 16:30

Mangling: Add a mangling for back deployment fallback functions and u…

ed6d69f

…pdate the back deployment thunk mangling to use the same prefix.

NFC: Remove out of date FIXME in back-deploy-attr.swift test.

123f953

tshortli force-pushed the back-deploy-thunk branch from af76a8d to 81edf79 Compare February 19, 2022 00:30

tshortli force-pushed the back-deploy-thunk branch from 81edf79 to 201381a Compare February 24, 2022 01:38

SILGen: Emit a fallback variant of a back deployable function and cal…

5339dae

…l it when the original function is not available at run time.

tshortli force-pushed the back-deploy-thunk branch from 201381a to 5339dae Compare February 24, 2022 02:07

tshortli requested a review from slavapestov February 24, 2022 02:17

tshortli merged commit 9e4ba44 into swiftlang:main Feb 26, 2022

tshortli deleted the back-deploy-thunk branch March 11, 2022 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit and call thunks for back deployed functions #41416

Emit and call thunks for back deployed functions #41416

tshortli commented Feb 16, 2022 •

edited

Loading

tshortli commented Feb 18, 2022

slavapestov left a comment

slavapestov Feb 18, 2022

tshortli Feb 19, 2022

slavapestov Feb 18, 2022

tshortli Feb 18, 2022

slavapestov Feb 18, 2022

tshortli Feb 19, 2022

slavapestov Feb 19, 2022

tshortli Feb 24, 2022

slavapestov Feb 18, 2022

tshortli Feb 18, 2022 •

edited

Loading

slavapestov Feb 18, 2022

tshortli Feb 18, 2022

slavapestov Feb 18, 2022

tshortli Feb 18, 2022

tshortli Feb 22, 2022

tshortli Feb 24, 2022

slavapestov Feb 18, 2022

tshortli Feb 18, 2022

tshortli Feb 19, 2022

slavapestov Feb 18, 2022

tshortli Feb 19, 2022

slavapestov Feb 18, 2022

tshortli Feb 19, 2022

slavapestov Feb 18, 2022

tshortli Feb 18, 2022

tshortli Feb 19, 2022

tshortli commented Feb 19, 2022

tshortli commented Feb 24, 2022

nkcsgexi commented Feb 26, 2022



		// Back deployment support functions are not in the vtable.
		if (backDeploymentKind != SILDeclRef::BackDeploymentKind::None)

Emit and call thunks for back deployed functions #41416

Emit and call thunks for back deployed functions #41416

Conversation

tshortli commented Feb 16, 2022 • edited Loading

tshortli commented Feb 18, 2022

slavapestov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshortli Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshortli commented Feb 19, 2022

tshortli commented Feb 24, 2022

nkcsgexi commented Feb 26, 2022

tshortli commented Feb 16, 2022 •

edited

Loading

tshortli Feb 18, 2022 •

edited

Loading