Add support for the early serialization of SIL modules using "high-level" SIL and make it the default #12427
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
slavapestov
reviewed
Oct 13, 2017
lib/SILGen/SILGenThunk.cpp
Outdated
| @@ -156,6 +156,13 @@ void SILGenFunction::emitCurryThunk(SILDeclRef thunk) { | |||
| SILValue toFn = getNextUncurryLevelRef(*this, vd, thunk, | |||
| selfArg, subs); | |||
|
|
|||
| // A curry thunk is serialized only if the target callee is serialized. | |||
| if (auto *FRI = dyn_cast<FunctionRefInst>(toFn)) { | |||
| if (FRI->getReferencedFunction()->isSerialized() == | |||
There was a problem hiding this comment.
Cn you move this elsewhere into emitCurryThunk() etc?
There was a problem hiding this comment.
Sorry, I do not quite understand what you mean. It is in the emitCurryThunk already.
There was a problem hiding this comment.
What was the bug that prompted this change? I'm not convinced it is correct.
Remember that the serialized flag has three states:
- not serialized
- serializable
- serialized
"Serializable" means that it is serialized only if referenced from another serialized function. Since curry thunks have shared linkage, they need to be serialized if referenced from another serialized function (ie, the caller). It doesn't matter if the callee is serialized or not. Consider the case where the callee is public and not serialized; the thunk has shared linkage, which your change will make not serialized. Then if I use the curry thunk from a serialized caller, it will blow up.
public struct S {
public func f() {}
}
@_inlineable public func g() {
print(S.f)
}
There was a problem hiding this comment.
The bug was that the curry thunk was marked serialized but the actual function that it calls was marked as NotSerialized.
There was a problem hiding this comment.
I'm all open to suggestions regarding how to fix it in a proper way.
There was a problem hiding this comment.
My naive idea was that if the target function is NotSerialized, then we cannot mark the curry thunk Serialized
There was a problem hiding this comment.
Can you come up with a reduced test case since this bug is not really related to your PR?
Serialized functions can reference non-serialized functions if they're public. So a curry thunk for anything public should be serializable since it can be serialized.
However, curry thunks for internal things should not be serializable. Was this the problem?
There was a problem hiding this comment.
May be. I need to reproduce it again to come up with a small test-case. I haven't created one when I hit it ;-(
slavapestov
reviewed
Oct 13, 2017
lib/SIL/SILModule.cpp
Outdated
|
|
||
| /// Removes [serialized] from all functions. This allows for more | ||
| /// optimizations and for a better dead function elimination. | ||
| static void removeSerializedFlagFromAllFunctions(SILModule &M) { |
There was a problem hiding this comment.
I would move this into the SIL pass; I would not expect SILModule::serialize() itself to have any side effects on the module's declarations, for example I might call it during debugging in the middle of the pipeline.
There was a problem hiding this comment.
OK. I'll move it back. It was there already ;-)
swiftix
force-pushed
the
sil-serialization-before-optimizations5
branch
from
9a2ee7e to
92f3ae0
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
swiftix
force-pushed
the
sil-serialization-before-optimizations5
branch
from
6a8b0c2 to
b383185
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux |
slavapestov
reviewed
Oct 14, 2017
| @@ -732,12 +732,14 @@ SILFunction *swift::getEligibleFunction(FullApplySite AI, | |||
| // A non-fragile function may not be inlined into a fragile function. | |||
| if (Caller->isSerialized() && | |||
| !Callee->hasValidLinkageForFragileInline()) { | |||
| #if 0 | |||
| if (!Callee->hasValidLinkageForFragileRef()) { | |||
There was a problem hiding this comment.
Please don't remove this check. It catches cases where serializable functions reference non-public things.
There was a problem hiding this comment.
It is only for now, to reproduce the failure.
There was a problem hiding this comment.
BTW, this is what fails due to public_external [serialized] functions as I explained in chat.
There was a problem hiding this comment.
I have a public_external function from the stdlib which was imported. It calls another public_external function. Both are public_external [serialized]. We specialize the callee and give it a shared linkage. And we don't mark it [serialized]. Now we have a public_external [serialized] function that calls a shared non-serialized function. Later on, we assert in the code snippet above that I disabled temporarily.
There was a problem hiding this comment.
@slavapestov What should be the right fix for such a case in your opinion?
There was a problem hiding this comment.
Please don't remove this check. It catches cases where serializable functions reference non-public things.
BTW, don't we catch it in SILVerifier anyways?
There was a problem hiding this comment.
The failure here https://ci.swift.org/job/swift-PR-Linux-smoke-test/1717/ seems to run into a manifestation of the same issue. We specialize and replace the apply in the [serialized] caller function to call the shared non-serialized specialized callee function.
There was a problem hiding this comment.
Specializations of public serialized functions should always be shared serialized.
There was a problem hiding this comment.
And yes we will ultimately catch it in the verifier but it's better to catch it sooner, closer to the original problem.
|
@swiftix I suspect there is a bug elsewhere in the optimizer that is causing it to inline something that it shouldn't, and it just happens to involve curry thunks. I would suggest removing the hacks, coming up with a reduced test case and trying to fix the underlying problem. |
|
@swiftix I see this code in Generics.cpp: Any ideas what's going wrong? If we specialize the same function F from a serialized context and a non-serialized contexts, the two specializations should get different manglings, too. But it might be worth double checking that. |
So it even has Anyway I'm sure you'll figure it out :) |
The `serialize` method can be called multiple times, but it will perform the actual serialization only the first time. By means of this API we get the flexibility to serialize the SILModule not only after all the optimizations, but e.g. at any time during optimizations.
swiftix
force-pushed
the
sil-serialization-before-optimizations5
branch
from
b383185 to
2423e9e
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci Please test source compatibility |
1 similar comment
|
@swift-ci Please test source compatibility |
|
@swift-ci please smoke test compiler performance |
1 similar comment
|
@swift-ci please smoke test compiler performance |
swiftix
force-pushed
the
sil-serialization-before-optimizations5
branch
from
2423e9e to
53754a7
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please benchmark |
1 similar comment
|
@swift-ci please benchmark |
|
Build failed |
Build comment file:Optimized (O)Regression (9)
Improvement (19)
No Changes (306)
Unoptimized (Onone)Regression (34)
Improvement (2)
No Changes (298)
Hardware Overview |
|
@slavapestov This version of the PR seems to pass all tests and does not contain any unrelated changes for thunks, etc. OK to merge? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Serialize SIL right before the performance inlining. The aim is to serialize a "high-level" SIL, which still contains all the calls of @_semantics functions. This preserves more information and allows for better optimizations of the clients.
This mode of serialization becomes the new default.