Always link SIL for partial modules in SILGen

[hamishknight]

Previously we would only link .sib partial modules in SILGen, with other kinds of serialized ASTs being linked just before the SILOptimizer if -sil-merge-partial-modules was specified. However linking always seems to be the desired behaviour, so adjust SILGen to always link SIL for all serialized AST inputs.

This allows us to remove some duplicated linking code from the SIL tools and will let us remove the -sil-merge-partial-modules frontend flag in favour of -merge-modules.

[slavapestov]

@hamishknight Thanks! In case you're wondering, I added the flag in case the SIL linking in merge-modules exposed problems. For a long time we didn't link SIL at all in this stage, so we would drop it all on the floor. Removing the flag makes sense since nowadays you will get incorrect semantics with this turned off anyway -- @_alwaysEmitIntoClient functions as well as default arguments must have their SIL preserved since they don't have public entry points.

[hamishknight]

@slavapestov Thanks for the background! I was also wondering about whether we could remove the MergePartialModules SIL option (as linking will now always happen with serialized inputs, even if -merge-modules isn't specified – though I don't think this use ever comes up in practice).

It seems like its use in DeserializeSIL could be replaced with a more general check for whether the function is being deserialized into the same module. Though I'm not sure about its use in SILVerifier, do you recall what case this is handling? No tests seem to fail if it's removed.

[slavapestov]

The idea with that option is that when we're merging partial modules, we don't want to deserialize bodies of functions in other modules. While it shouldn't affect correctness, it might impact performance. Perhaps we should add a counter-based test to ensure that we don't do this.

I think the use in DeserializeSIL has to stay. We don't know if the function is being deserialized into the same module or not. Normally when I deserialize a function, the linkage changes, for example a public function deserializes as public_external, and public_non_abi deserializes as shared. The distinction is that we wish to make the SIL available for optimization purposes, but we don't actually want IRGen to re-emit these deserialized functions. IRGen knows to skip a public_external function, even if it has a body.

The latter should be covered by tests; again, if it isn't, we should add new tests.

[hamishknight]

I think the use in DeserializeSIL has to stay. We don't know if the function is being deserialized into the same module or not.

@slavapestov Wouldn't getFile()->getParentModule() == SILMod.getSwiftModule() be sufficient to check that?

Normally when I deserialize a function, the linkage changes, for example a public function deserializes as public_external, and public_non_abi deserializes as shared.

Right, but for merge modules as far as I can tell we should always preserve the linkage of what we deserialize. So if I understand things correctly, anything that merge modules would trip the SILVerifier for outside of "single function" mode would also be something that an individual partial module could trip it for. Therefore I'm tempted to think we don't need to run the SILVerifier in "single function" mode for merge modules. What do you think?

The latter should be covered by tests; again, if it isn't, we should add new tests.

Sure, I'd be happy to look into adding a test for that :)

[hamishknight]

Let's see if anything breaks

@swift-ci please test

[hamishknight]

@swift-ci please test source compatibility

Results: Debug, Release

[hamishknight]

Right, but for merge modules as far as I can tell we should always preserve the linkage of what we deserialize. So if I understand things correctly, anything that merge modules would trip the SILVerifier for outside of "single function" mode would also be something that an individual partial module could trip it for. Therefore I'm tempted to think we don't need to run the SILVerifier in "single function" mode for merge modules. What do you think?

Ah okay, I realise now what you were referring to with regard to merge-modules is the fact that we skip the mandatory linking pass, so we could encounter a shared_external function with no body. Fortunately though the merge-modules check in the SILVerifier is subsumed by the check for F.getModule().getStage() >= SILStage::Canonical, so it should still be okay to remove the special case for merge-modules.

[hamishknight]

Ping @slavapestov @gottesmm

[hamishknight]

@swift-ci please test