Remove the -sil-serialize-all option #12256
Merged
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 Slava, do you want to have a look? |
|
@swift-ci please test |
|
Build failed |
slavapestov
reviewed
Oct 4, 2017
include/swift/SIL/SILModule.h
Outdated
| @@ -323,7 +323,7 @@ class SILModule { | |||
| } | |||
|
|
|||
| /// Returns true if everything in this SILModule is being serialized. | |||
| bool isWholeModuleSerialized() const { return Options.SILSerializeAll; } | |||
| bool isWholeModuleSerialized() const { return false; } | |||
There was a problem hiding this comment.
Are you going to remove this, and ResilienceStrategy::Fragile, etc?
There was a problem hiding this comment.
Yes, I'm going to remove it as well. There is no need for it anymore.
There was a problem hiding this comment.
Also makeModuleFragile() in SILGen
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
Build failed |
|
StdlibUnittest was still using -sil-serialize-all in order to make sure that things actually get specialized. Are we going to do something about that? cc @moiseev |
|
@jrose-apple I don't think so. Can you un-revert my PR so that this one can apply cleanly? |
slavapestov
reviewed
Oct 4, 2017
| @@ -258,8 +258,6 @@ ModuleDecl *CompilerInstance::getMainModule() { | |||
|
|
|||
| if (Invocation.getFrontendOptions().EnableResilience) | |||
| MainModule->setResilienceStrategy(ResilienceStrategy::Resilient); | |||
| else if (Invocation.getSILOptions().SILSerializeAll) | |||
| MainModule->setResilienceStrategy(ResilienceStrategy::Fragile); | |||
There was a problem hiding this comment.
You forgot to remove ResiienceStrategy::Fragile
There was a problem hiding this comment.
@slavapestov In cases like below, should we check instead for SILSerializeWitnessTables or remove them alltogether?
if (isProtocolConformanceKind(getKind())) {
auto conformance = getProtocolConformance();
auto conformanceModule = conformance->getDeclContext()->getParentModule();
auto isCompletelySerialized = conformanceModule->getResilienceStrategy() ==
ResilienceStrategy::Fragile;
// The conformance is fragile if it is in a -sil-serialize-all module.
return isCompletelySerialized;
}There was a problem hiding this comment.
I think you should check your flag yes.
There was a problem hiding this comment.
@slavapestov Hmm. There is a small problem here. The code above is inside LinkEntity::isFragile, which does not contain any reference to the SILModule, which has SILSerializeWitnessTables flag. It can only access ModuleDecl through conformance->getDeclContext()->getParentModule().
How do we solve this in the cleanest way? Should we add an IGM argument to the function? Should we add a flag for -sil-serialize-witness-tables and -sil-serialize-vtables to the ModuleDecl or may be even have a reference from ModuleDecl to SILOptions?
BTW, why don't ModuleDecls have a back-reference to the SILModules? It could be useful for such cases...
There was a problem hiding this comment.
OK. I moved SILSerializeWitnessTables and SILSerializeVTables into LanguageOptions, so that they are accessible from ASTContext and ModuleDecl.
…only")" With -sil-serialize-all gone, this no longer means anything; just don't declare the function as @_inlineable instead. Fixes <rdar://problem/34564380>.
swiftix
force-pushed
the
sil-serialize-all-improvments
branch
from
22b4205 to
3ade0a9
Compare
|
@swift-ci please clean smoke test |
|
@swift-ci please clean test |
1 similar comment
|
@swift-ci please clean test |
|
Build failed |
swiftix
force-pushed
the
sil-serialize-all-improvments
branch
from
3ade0a9 to
6c85fff
Compare
|
@swift-ci please clean smoke test |
1 similar comment
|
@swift-ci please clean smoke test |
|
@swift-ci please clean smoke test Linux |
|
@swift-ci please clean test Linux |
|
@swift-ci please clean test |
1 similar comment
|
@swift-ci please clean test |
|
@swift-ci please smoke benchmark |
1 similar comment
|
@swift-ci please smoke benchmark |
|
Build failed |
|
Build failed |
Build comment file:Optimized (O)Regression (10)
Improvement (16)
No Changes (307)
Unoptimized (Onone)Regression (30)
Improvement (13)
No Changes (290)
Hardware Overview |
Only vtables of public classes should be serialized. And only entries for public or serialized methods should be serialized.
And remove all the code that is dead because of it.
swiftix
force-pushed
the
sil-serialize-all-improvments
branch
from
f824999 to
06efb77
Compare
|
@swift-ci please clean smoke test |
1 similar comment
|
@swift-ci please clean smoke test |
|
@swift-ci please clean test |
|
Build failed |
|
@swift-ci Please test source compatibility |
|
@slavapestov The tests seem to pass finally, at least the smoke tests. OK to merge once the full tests are green? Or do we want to anything else to this PR? |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
@swift-ci please test |
|
@swift-ci please clean test |
|
Looks good to go! |
|
Looks like this is still causing problems somehow in https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/180/, under optimized testing (something "please [clean] test" doesn't do). @swiftix, are you going to have time to look at this this morning or should I revert again? |
|
|
Or did you or Slava already get to it? |
|
@jrose-apple I'll try to have a look |
|
@jrose-apple I tried to reproduce locally using |
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.
No description provided.