Skip to content
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

[SIL] Add [serialized_for_package] to control package-wide resilience domain in Package-CMO. #73566

Merged
merged 3 commits into from May 16, 2024

Conversation

elsh
Copy link
Contributor

@elsh elsh commented May 10, 2024

This PR adds a new attribute [serialized_for_package] to allow
more fine-control over package-wide resilience domain if
Package CMO is enabled.

The purpose of the attribute includes:

  • Indicates that certain types such as loadable types are
    allowed in serialized functions in resiliently built module
    if the optimization is enabled, which are otherwise disallowed.
  • Used during SIL deserialization to determine whether such
    functions are allowed.
  • Used to determine if a callee can be inlined into a caller
    that's serialized without package-cmo, e.g. with an explicit
    annotation like @inlinable, where the callee was serialized
    due to package-cmo.

Resolves rdar://127870822&127321129&127308331

@aschwaighofer
Copy link
Member

We should block inlining a [serialized] [serialized_for_package] function into a [serialized]-only function until the [serialized] attribute is removed.

Otherwise, you end up with the code originating from a [serialized_for_package] function that assumes loadability in a [serialized] function (without the serialized_for_package) attributed and the verifier will complain.

We achieve the analogous thing for serialized and not-serialized functions here:

if (Caller->isSerialized() &&
!Callee->hasValidLinkageForFragileInline()) {
if (!Callee->hasValidLinkageForFragileRef()) {

bool hasValidLinkageForFragileInline() const { return isSerialized(); }

package-wide resilience domain if Package CMO is enabled.

The purpose of the attribute includes:
- Indicates that certain types such as loadable types are
allowed in serialized functions in resiliently built module
if the optimization is enabled, which are otherwise disallowed.
- Used during SIL deserialization to determine whether such
functions are allowed.
- Used to determine if a callee can be inlined into a caller
that's serialized without package-cmo, e.g. with an explicit
annotation like @inlinable, where the callee was serialized
due to package-cmo.

Resolves rdar://127870822
@elsh
Copy link
Contributor Author

elsh commented May 15, 2024

@swift-ci smoke test

@elsh elsh changed the title [SIL] Add a new attribute [serialized_for_package]. [SIL] Add [serialized_for_package] to control package-wide resilience domain in Package-CMO. May 15, 2024
@slavapestov
Copy link
Member

We should block inlining a [serialized] [serialized_for_package] function into a [serialized]-only function until the [serialized] attribute is removed.

I think the bits should be mutually exclusive no? So both should not be set at once. [serialized] is the stonger condition so if its set, it implies the same invariants as [serialized_for_package] but adds more.

@elsh elsh requested review from hborla and xedin as code owners May 15, 2024 22:39
@elsh
Copy link
Contributor Author

elsh commented May 15, 2024

@swift-ci smoke test

-Fix SILDeclRef getLinkageLimit() for GlobalAccessor to return
Limit::None if bypassResilienceInPackage is enabled.
@elsh
Copy link
Contributor Author

elsh commented May 15, 2024

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented May 16, 2024

@swift-ci smoke test

@@ -162,6 +162,11 @@ enum IsSerialized_t : unsigned char {
IsSerialized
};

enum IsSerializedForPackage_t : unsigned char {
Copy link
Member

Choose a reason for hiding this comment

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

I agree with Slava. You should combine both enums, e.g.

enum IsSerialized_t : unsigned char {
  IsNotSerialized,
  IsSerialized,
  IsSerializedForPackage
};

Copy link
Member

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

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

I think we can serialization_t tri-state enum as a follow up PR that also addresses inlinability

@aschwaighofer
Copy link
Member

we can address ...

@elsh elsh merged commit 1257db7 into main May 16, 2024
3 checks passed
@elsh elsh deleted the elsh/sil-new-attr branch May 16, 2024 17:17
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.

None yet

4 participants