Skip to content

Add -internalize-at-link flag to allow dead stripping and VFE across modules at link time #39214

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
Sep 21, 2021

Conversation

kubamracek
Copy link
Contributor

The first commit in the PR is being reviewed at #39128. Suggestion for reviewers is to only look at the second commit here.

  • Under -internalize-at-link, stop unconditionally marking all globals as used.
  • Under -internalize-at-link, restrict visibility of vtables to linkage unit.
  • Emit virtual method thunks for cross-module vcalls when VFE is enabled.
  • Use thunks for vcalls across modules when VFE is enabled.
  • Adjust TBDGen to account for virtual method thunks when VFE is enabled.
  • Add an end-to-end test case for cross-module VFE.

@@ -1782,7 +1791,13 @@ namespace {
// method for external clients.

// Emit method dispatch thunk.
if (hasPublicVisibility(fn.getLinkage(NotForDefinition))) {
bool shouldEmitDispatchThunk = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this logic, which is the same as the previous change, be extracted into a utility function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't entirely identical, so I've instead simplified it into just an or-d expression inside the if.

@kubamracek kubamracek force-pushed the vfe2 branch 3 times, most recently from 2c2c144 to 97d3f16 Compare September 15, 2021 14:37
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

LGTM

if (IGM.hasResilientMetadata(classDecl, ResilienceExpansion::Maximal)) {
shouldUseDispatchThunk = true;
} else if (IGM.getOptions().VirtualFunctionElimination) {
// For VFE, use a thunk if the target class is in another module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment why this is needed?

@kubamracek kubamracek force-pushed the vfe2 branch 2 times, most recently from 71f21d2 to 69bef5e Compare September 21, 2021 14:05
…modules at link time

- Under -internalize-at-link, stop unconditionally marking all globals as used.
- Under -internalize-at-link, restrict visibility of vtables to linkage unit.
- Emit virtual method thunks for cross-module vcalls when VFE is enabled.
- Use thunks for vcalls across modules when VFE is enabled.
- Adjust TBDGen to account for virtual method thunks when VFE is enabled.
- Add an end-to-end test case for cross-module VFE.
@kubamracek
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b7cb377

@kubamracek
Copy link
Contributor Author

@swift-ci please test Linux platform

@kubamracek kubamracek merged commit 9fb54e9 into swiftlang:main Sep 21, 2021
@kubamracek kubamracek deleted the vfe2 branch September 21, 2021 19:26
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