Skip to content

SILGen: Skip emitting distributed thunks for skipped functions #68917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Oct 2, 2023

Follow up to #68760 to fix a failed assertion when building the swift-distributed-actors project.

@tshortli tshortli requested a review from ktoso as a code owner October 2, 2023 18:06
@tshortli
Copy link
Contributor Author

tshortli commented Oct 2, 2023

@swift-ci please smoke test

// CHECK-SKIP-NONINLINE-LABEL: sil [serialized] [distributed] [ossa] @$s38distributed_thunk_skip_function_bodies2DAC13inlinableFuncyyF : $@convention(method) (@guaranteed DA) -> () {
// CHECK-SKIP-NONINLINE: function_ref @$s38distributed_thunk_skip_function_bodies9blackHoleyyxlF
// CHECK-SKIP-NONINLINE: } // end sil function '$s38distributed_thunk_skip_function_bodies2DAC13inlinableFuncyyF'
@inlinable public distributed func inlinableFunc() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does an @inlinable distributed func even make sense? I'm sort of surprised this is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's okay because we inject an implicit if/else that either dispatches remove or goes though the local logic so inlining shouldn't break thunking but it would be good to add a test for that to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should be fine to inline distributed thunks, they're the if-remote-do-this code

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@xedin
Copy link
Contributor

xedin commented Oct 2, 2023

@swift-ci please test source compatibility debug

@tshortli tshortli changed the title SILGen: Skip emitting distributed thunks for skipped functions. SILGen: Skip emitting distributed thunks for skipped functions Oct 2, 2023
@tshortli tshortli merged commit fa4a0ab into swiftlang:main Oct 2, 2023
@tshortli tshortli deleted the skipped-distributed-actor-thunk branch October 2, 2023 22:44
tshortli added a commit to tshortli/swift that referenced this pull request Oct 26, 2023
Generalizes swiftlang#68917 to cover accessors in
addition to methods.

Resolves rdar://117226130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants