Support inlinability with [serialized_for_package] #73754

elsh · 2024-05-20T18:00:58Z

SIL function can be serialized with different kinds: [serialized] or
[serialized_for_package] if Package CMO is enabled. The latter kind
allows a function to be serialized even if it contains loadable types,
if Package CMO is enabled.

The tri-state serialization kind requires validating inlinability
depending on the serialization kinds of callee vs caller; e.g. if the
callee is [serialized_for_package], the caller must be not [serialized].

Package CMO sets [serialized_for_package] to SIL function, global var,
v-table, and witness-table, if inlinable.

Renamed IsSerialized_t as SerializedKind_t and added IsSerializedForPackage.
Updated various call sites with getSerializedKind().
SILFunction::canBeSerializedIntoCaller handles inlinability checks b/t caller and callee.

Resolves rdar://128406520

elsh · 2024-05-20T18:09:23Z

@swift-ci smoke test

nkcsgexi · 2024-05-20T21:40:32Z

Thank you! This makes more sense than maintaining two booleans.

elsh · 2024-05-21T00:41:29Z

@swift-ci smoke test

elsh · 2024-05-21T09:42:13Z

@swift-ci smoke test

elsh · 2024-05-22T00:47:50Z

@swift-ci test

elsh · 2024-05-22T07:17:16Z

@swift-ci test

elsh · 2024-05-22T11:01:42Z

@swift-ci test

elsh · 2024-05-22T11:17:37Z

@swift-ci test

elsh · 2024-05-22T16:28:33Z

@swift-ci test

elsh · 2024-05-22T18:02:30Z

@swift-ci test

elsh · 2024-05-23T01:47:32Z

@swift-ci test

include/swift/SIL/SILFunction.h

lib/SIL/IR/SILFunction.cpp

elsh · 2024-05-23T19:08:38Z

@swift-ci test

aschwaighofer · 2024-05-23T19:13:29Z

include/swift/SIL/SILFunction.h


  /// Returns true if this function can be referenced from a fragile function
  /// body.
-  bool hasValidLinkageForFragileRef() const;
+  bool hasValidLinkageForFragileRef(


We should remove the optional here and always pass the referencing entity's SerializationKind_t.

It can come from the enclosing/referencing abstraction: function, v-table, witness table, property, movedeinit.

aschwaighofer · 2024-05-23T19:25:25Z

lib/SIL/IR/SILFunction.cpp

+*/
+bool SILFunction::canBeInlinedIntoCaller(
+      std::optional<SerializedKind_t> callerSerializedKind,
+      bool assumeFragileCaller) const {


I don't understand the need of the boolean assumeFragileCaller.

If the caller is fragile (i.e serialized) this should be communicated via passing SerializedKind_t::Serialized

That argument should not be optional.

aschwaighofer · 2024-05-23T20:23:11Z

I think the APIs should be:

bool hasValidLinkageForFragileRef(SerializationKind_t fromContext);
bool canBeInlinedIntoCaller(SerializationKind_t callerSerialization);

hasValidLinkageForFragileRef probably should become:

bool SILFunction::hasValidLinkageForFragileRef(SerializationKind_t fromContext) const {                        
  ....                                                         
                                                                                
  // If we can inline it, we can reference it.
  assert(fromContext != SerializationKind_t::NotSerialized);                                  
  if (canBeInlinedIntoCaller(fromContext))                                        
    return true;

And we pass down the context from the enclosing entity (I think you are already doing that in some places of your patch). Some examples follow.

SILVerifier.cpp (here the context/ referencing entity is the SILWitnessTable):

void SILWitnessTable::verify(const SILModule &M) const {                        
  if (!verificationEnabled(M))                                                  
    return;                                                                     
                                                                                
  if (isDeclaration())                                                          
    assert(getEntries().empty() &&                                              
           "A witness table declaration should not have any entries.");         
                                                                                
  for (const Entry &E : getEntries())                                           
    if (E.getKind() == SILWitnessTable::WitnessKind::Method) {                  
      SILFunction *F = E.getMethodWitness().Witness;                            
      if (F) {                                                                  
        // If a SILWitnessTable is going to be serialized, it must only         
        // reference public or serializable functions.                          
 +       if (!isNotSerialized()) {                                                   
 +         assert(F->hasValidLinkageForFragileRef(this->getSerializationKind()) &&                           
                 "Fragile witness tables should not reference "                 
                 "less visible functions.");

Devirtualizer.cpp (here the context/referencing entity is the caller function):

+    if (!applySite.getFunction()->isNotSerialized()) {                                
  // function_ref inside fragile function cannot reference a private or       
  // hidden symbol.                                                           
+    if (!f->hasValidLinkageForFragileRef(applySite.getFunction->getSerializationKind()))                                     
    return false;                                                             
}

[serialized_for_package] if Package CMO is enabled. The latter kind allows a function to be serialized even if it contains loadable types, if Package CMO is enabled. Renamed IsSerialized_t as SerializedKind_t. The tri-state serialization kind requires validating inlinability depending on the serialization kinds of callee vs caller; e.g. if the callee is [serialized_for_package], the caller must be _not_ [serialized]. Renamed `hasValidLinkageForFragileInline` as `canBeInlinedIntoCaller` that takes in its caller's SerializedKind as an argument. Another argument `assumeFragileCaller` is also added to ensure that the calle sites of this function know the caller is serialized unless it's called for SIL inlining optimization passes. The [serialized_for_package] attribute is allowed for SIL function, global var, v-table, and witness-table. Resolves rdar://128406520

elsh · 2024-05-23T23:05:40Z

bool SILFunction::hasValidLinkageForFragileRef(SerializationKind_t fromContext) const {
....

// If we can inline it, we can reference it.
assert(fromContext != SerializationKind_t::NotSerialized);
if (canBeInlinedIntoCaller(fromContext))
return true;

Making the arg non-optional requires passing in caller info to hasValidLinkageForFragileRef which is called in several spots and will require more changes. As this PR is already too large, I'd like to address the feedback in a separate PR along with changes to SIL optimization passes that call the function.

elsh · 2024-05-23T23:06:50Z

@swift-ci test

aschwaighofer · 2024-05-24T14:02:56Z

include/swift/SIL/SILDeclRef.h

@@ -384,7 +384,11 @@ struct SILDeclRef {
  /// True if the function should be treated as transparent.
  bool isTransparent() const;
  /// True if the function should have its body serialized.
-  IsSerialized_t isSerialized() const;
+  bool isSerialized() const;


In a follow-up PR we should also introduce a predicate: bool isAnySerializedKind().

The places where we now have to write double negation !isNotSerialized() are really hard to read. At least for my senile brains.

Already on it in an upcoming PR; I agree it's illegible for me too :)

aschwaighofer

Some changes here are a regression in correctness running in package CMO mode until we follow up with a patch that properly passes the serialization kind around in all places where we now pass assumeFragileCaller.

That is fine with me in the interest of smaller changes, with the understanding that we will have to follow-up with those changes.

aschwaighofer · 2024-05-24T15:39:02Z

lib/SIL/IR/SILFunction.cpp

@@ -554,9 +553,6 @@ ResilienceExpansion SILFunction::getResilienceExpansion() const {
  // attribute is preserved if the current module is in
  // the same package, thus should be in the same resilience
  // domain.
-  if (isSerializedForPackage() == IsSerializedForPackage)


Wait I don't understand why these lines were removed.

The resilience expansion inside of a [serialized_for_package] function is Maximal -- we allow loadable types.

Right, in the line below, maximal is returned if [serialized_for_package].

Oh, right isSerialized is only [serialized] not [serialized_for_package]

elsh · 2024-05-24T17:25:50Z

Some changes here are a regression in correctness running in package CMO mode until we follow up with a patch that properly passes the serialization kind around in all places where we now pass assumeFragileCaller.

That is fine with me in the interest of smaller changes, with the understanding that we will have to follow-up with those changes.

The assumeFragileCaller arg was actually passed to keep the existing behavior as much as possible except when necessary to pass a caller's serializedKind to keep the previous state of Package CMO. In the upcoming PR, the arg will be removed and inlining and other relevant passes will take advantage of the new API.

#73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

#74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

#74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

#74531 #74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh requested a review from kavon as a code owner May 20, 2024 18:00

elsh requested review from slavapestov and aschwaighofer May 20, 2024 18:03

elsh force-pushed the elsh/serialized_kind branch from dd594f8 to 4152f34 Compare May 20, 2024 18:42

elsh requested a review from nkcsgexi May 20, 2024 18:57

elsh force-pushed the elsh/serialized_kind branch 2 times, most recently from 34a67f4 to cf1a94c Compare May 21, 2024 00:41

elsh force-pushed the elsh/serialized_kind branch from cf1a94c to 70c6446 Compare May 21, 2024 09:41

elsh force-pushed the elsh/serialized_kind branch 2 times, most recently from f2ae567 to d991810 Compare May 22, 2024 00:14

elsh force-pushed the elsh/serialized_kind branch from d991810 to ecafc6a Compare May 22, 2024 07:16

elsh force-pushed the elsh/serialized_kind branch from 27cf40e to cc3ca1c Compare May 22, 2024 11:17

elsh force-pushed the elsh/serialized_kind branch from d09bf98 to 0e90551 Compare May 22, 2024 18:02

elsh force-pushed the elsh/serialized_kind branch from 0e90551 to 9f5d5ee Compare May 23, 2024 01:46

aschwaighofer reviewed May 23, 2024

View reviewed changes

include/swift/SIL/SILFunction.h Outdated Show resolved Hide resolved

aschwaighofer reviewed May 23, 2024

View reviewed changes

lib/SIL/IR/SILFunction.cpp Outdated Show resolved Hide resolved

aschwaighofer reviewed May 23, 2024

View reviewed changes

elsh force-pushed the elsh/serialized_kind branch from 36d7fb0 to 5ccc4cd Compare May 23, 2024 22:54

This was referenced May 24, 2024

[6.0] [PackageCMO] Support inlinability with [serialized_for_package] #73876

Closed

Add a check for serialized_for_package in MandatoryInlining #73715

Closed

aschwaighofer reviewed May 24, 2024

View reviewed changes

aschwaighofer approved these changes May 24, 2024

View reviewed changes

aschwaighofer reviewed May 24, 2024

View reviewed changes

elsh merged commit 9392a69 into main May 24, 2024
5 checks passed

elsh deleted the elsh/serialized_kind branch May 24, 2024 17:13

elsh added a commit that referenced this pull request May 29, 2024

Package CMO containing the following PRs.

fe44859

#73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh mentioned this pull request May 29, 2024

[6.0] Support Package CMO #73993

Merged

elsh added a commit that referenced this pull request Jun 5, 2024

Package CMO containing the following PRs.

f5508cf

#74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh added a commit that referenced this pull request Jun 17, 2024

Package CMO containing the following PRs.

db1a809

#74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh added a commit that referenced this pull request Jun 18, 2024

Package CMO containing the following PRs.

1fbfdfa

#74531 #74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh added a commit that referenced this pull request Jun 18, 2024

Package CMO containing the following PRs.

c09340d

#74531 #74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

elsh added a commit that referenced this pull request Jun 19, 2024

Package CMO containing the following PRs.

4908327

#74531 #74468 #74416 #74070 #73902 #73754 #73717 #72606 #73566 #73478 #73686 #72937 #73708 #73751 #73794

Support inlinability with [serialized_for_package] #73754

Support inlinability with [serialized_for_package] #73754

Uh oh!

Conversation

elsh commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elsh commented May 20, 2024

Uh oh!

nkcsgexi commented May 20, 2024

Uh oh!

elsh commented May 21, 2024

Uh oh!

elsh commented May 21, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 22, 2024

Uh oh!

elsh commented May 23, 2024

Uh oh!

Uh oh!

Uh oh!

elsh commented May 23, 2024

Uh oh!

aschwaighofer May 23, 2024

Choose a reason for hiding this comment

Uh oh!

aschwaighofer May 23, 2024

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented May 23, 2024

Uh oh!

elsh commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elsh commented May 23, 2024

Uh oh!

aschwaighofer May 24, 2024

Choose a reason for hiding this comment

Uh oh!

elsh May 24, 2024

Choose a reason for hiding this comment

Uh oh!

aschwaighofer left a comment

Choose a reason for hiding this comment

Uh oh!

aschwaighofer May 24, 2024

Choose a reason for hiding this comment

Uh oh!

elsh May 24, 2024

Choose a reason for hiding this comment

Uh oh!

aschwaighofer May 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elsh commented May 24, 2024

Uh oh!

Uh oh!

elsh commented May 20, 2024 •

edited

Loading

elsh commented May 23, 2024 •

edited

Loading