Support generating loadable types in serialized function when package-cmo is enabled.

[elsh]

To support generating loadable types in a serialized function, return maximal resilience expansion in SILFunction::getResilienceExpansion() if package serialization is enabled for modules built resiliently.
This allows aggregate types to be generated as loadable SIL types which otherwise are address-only
in a serialized function. During type lowering, opaque flag setting is also skipped if package serialization
is enabled.

Resolves rdar://127400743

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[aschwaighofer]

Hmm, I think we are working around the fact that type lowering does not correctly answer the question whether a type is loadable in the context we are looking at.

Today, type lowering takes the type expansion context (resilience expansion, etc) into account. I think we need to add another context item: are we lowering for abstraction purposes (i.e. in SILGen to decide the ABI of function signatures and so parameters need the right abstraction) or are we later in the pipeline (in which case we should check whether we can ignore the resilience expansion if experimental-allow-non-resilient-access/experimental-package-bypass-resilience is enabled)

[elsh]

@swift-ci smoke test

[zoecarver]

It seems like the resilience of a type shouldn't change throughout the compilation. If resilience is disabled for package types, that should be represented throughout the whole compilation, right? The || isContextPostAbstractions seem to be compensating for the fact that type lowering doesn't represent types with bypassed resilience correctly. Can we just teach type lowering about package-bypass-resilience?

[elsh]

@swift-ci smoke test

[elsh]

@swift-ci smoke test

[aschwaighofer]

@zoecarver

It seems like the resilience of a type shouldn't change throughout the compilation.

Resilience does not change. But the context in which we are ask to "expand" a type changes. This is captured by the TypeExpansionContext/ResilienceExpansion abstractions. The TypeExpansionContext is consulted whenever we want to know what kind of abstraction pattern to access a type: e.g loadable or by-address, passing direct or indirect, ...

What is being proposed is to model the fact in the TypeExpansionContext that we are allowed to treat types as loadable inside of serialized (~ inlinable) function bodies under the right circumstances: experimental-allow-non-resilient-access/experimental-package-bypass-resilience even though the resilience expansion otherwise indicates a minimal expansion .

In the world where we don't use package-CMO and bypass-resilience a serialized (~ inlinable) function is not allowed to use a "loadable" abstraction pattern:

sil [serialize] @some_inlinable_function: @convention(thin) (@in ResilientTypeInCurrentModule) -> () {
bb(%addr : $*ResilientType):
    copy_addr %addr to %dest: $* ResilientTypeInCurrentModule // Okay
    %val = load %addr : $* ResilientTypeInCurrentModule // Not okay
    store %val to %dest :$* ResilientTypeInCurrentModule // Not okay
}

This is because this SIL could be still inlined into another module/binary that does not have access to type layout and so it is not okay to assume loadability.

However, it is okay once you remove the [serialize] attribute from the function, which happens after serializing the SIL of a module (before going to code generation). The resilience expansion of a SIL function changes from minimal to maximal based on the [serialize] attribute.

sil @some_inlinable_function: @convention(thin) (@in ResilientTypeInCurrentModule) -> () {
bb(%addr : $* ResilientTypeInCurrentModule):
    copy_addr %addr to %dest: $* ResilientTypeInCurrentModule // Okay
    %val = load %addr : $* ResilientTypeInCurrentModule // Now okay
    store %val to %dest :$* ResilientTypeInCurrentModule // Now okay
}

With package CMO and experimental-allow-non-resilient-access/experimental-package-bypass-resilience the above is okay even for a [serialize] function. Or at least that is what we desire.

sil [serialize] @some_inlinable_function: @convention(thin) (@in ResilientTypeInCurrentModule) -> () {
bb(%addr : $* ResilientTypeInCurrentModule):
    %val = load %addr : $* ResilientTypeInCurrentModule // Okay because we know the module we deserialize into has access to type layout.
    store %val to %dest :$* ResilientTypeInCurrentModule // Okay
}

However, when a module that imports another module in the same package that defines the resilient type decides the ABI of the function it still needs to treat it as address-only and pass by address.

sil @some_public_function : @convention(thin) (@in ResilientTypeFromTheSamePackage)
NOT:
sil @some_public_function : @convention(thin) (@owned ResilientTypeFromTheSamePackage)

This means there need to be two distinct states that TypeExpansionContext models:

The one that we use when we lower the signature to decide to pass indirectly for ABI purposes:

sil @some_public_function : @convention(this) (@in ResilientTypeFromTheSamePackage)

The other one that tells us it is okay to use a loadable access pattern inside the function body.

sil [serialize] @some_inlinable_function: @convention(thin) (@in ResilientType) -> () {
bb(%addr : $*ResilientType):
    %val = load %addr : $*ResilientType // Okay because we know the module we deserialize into has access to type layout.

[zoecarver]

Resilience does not change. But the context in which we are ask to "expand" a type changes.

I am still struggling with this a bit. Maybe I still don't understand these APIs, but it seems like one of these APIs is being used incorrectly or is implemented incorrectly.

Focusing on the overload of isResilient that does take a ResilienceExpansion: you're saying this overload should not be used to determine if a type is resilient? If so, maybe we can document that in a comment or even rename it? Regardless, it does seem to be used to determine if a type should be resilient, for example, in IRGen in TypeConverter::convertStructType we call isResilient(D, ResilienceExpansion::Maximal) which will return false for public struct types and influence how those types are lowered.

This is a huge problem for codegen, if we compile your above example on main:

// Core.swift
public struct X { }

public func test() -> X {
	X()
}

// Main.swift
import Core

@inlinable
public func caller() -> X {
  test()
}

// Client.swift
import Main

public func program() {
  _ = caller()
}

This produces the following codegen:

define swiftcc void @"$s6client7programyyF"() {
entry:
  tail call swiftcc void @"$s4Core4testAA1XVyF"(ptr noalias nocapture sret(%T4Core1XV) undef)
  %0 = tail call ptr @"$s4Core1XVWOh"(ptr undef)
  ret void
}

Those undefs are because we are trying to turn an empty explosion into a resilient type, because we now disagree on a type's resilience outside of a package resilience domain.

Concretely, my suggestions here are:

1. To update type lowering to understand this new flag (by updating handleResilience)
2. Update all overloads of isResilient to agree and constrain them to only return false for package decls (today they return false when this flag is on even for public decls).

[elsh]

Those undefs are because we are trying to turn an empty explosion into a resilient type, because we now disagree on a type's resilience outside of a package resilience domain.

Concretely, my suggestions here are:

1. To update type lowering to understand this new flag (by updating handleResilience)
2. Update all overloads of isResilient to agree and constrain them to only return false for package decls (today they return false when this flag is on even for public decls).

As discussed offline, clients that explicitly opt in to bypass resilience checks to Core types have access to the type layout and are required to be built together with Core, while clients that don't opt in should have indirect access. This is why isResilient() that determines resilience of the definition site needs to be unaltered. Altering will also have a negative impact on what's generated in .swiftinterface where types are expected be resilient.

[elsh]

The latest updates in this PR no longer involve changes to TypeExpansionContext.

[aschwaighofer]

Looks good. I assume we remove the serialized flag upon deserialization? Otherwise, we might run into issues with sil-verify-all.

[elsh]

Looks good. I assume we remove the serialized flag upon deserialization? Otherwise, we might run into issues with sil-verify-all.

Are you referring to serializePackageEnabled bit? The flag is still there. The removed part in checkResilience is no longer hit because when package serialization is enabled, the returned resilience expansion is now maximal.

[aschwaighofer]

I referred to the client module that imports the serialized sil. Will that also be compiled with allow-non-resilient-package access?

[aschwaighofer]

The override of ‘getResilienceExpansion’ will only work in modules that are compiled with ’experimental-allow-non-resilient-access’?

Once you deserialize the function into another module the override will only work if that importing module was also compiled with ‘experimental-allow-non-resilient-access’ or that module has removed the serialize flag upon importing?