Clean up SIL witness table deserialization #21476
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
…ration() All callers were doing the same thing here, so move it inside the function. Also, change getRootNormalConformance(), which is deprecated, to getRootConformance().
Note that SILModule::lookUpWitnessTable() only attempts to deserialize the witness table if we already have a declaration. In the SILLinkerVisitor, we would call lookUpWitnessTable(), see if it returned null, create a declaration, and if deserializeLazily is true, we would call lookUpWitnessTable() again; this time, there was a declaration, and we would deserialize the witness table. However, if there was already a witness table declaration when we first called lookUpWitnessTable(), we would trigger deserialization, even if deserializeLazily is false. Change the first call to pass false. I had to update a couple of tests; I believe they were already somewhat nonsensical.
This is actually NFC: We should have already deserialized everything we need at this point, and because of large loadable types and address lowering, deserializing more stuff in IRGen is not valid, and in fact we check for this and refuse to deserialize.
Should be NFC for now, since callers either assume the linker pass has run, or they manually call createWitnessTableDeclaration().
Now that SILModule::lookUpWitnessTable() can create a declaration if needed, we don't have to call it twice.
…ady exists I discovered this while testing the new commit, since it results in fewer witness tables getting pre-declared. There was a SIL test that relied on the old behavior; I'm going to explicitly declare those witness tables as public_external to preserve behavior.
…structions Another simplification made possible by the change to SILModule::lookUpWitnessTable().
The code becomes simpler if we fold it into its only caller.
slavapestov
requested review from
jckarter,
gottesmm,
marcrasi and
ravikandhadai
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
The change is pretty small but I split it up into multiple commits and tested each one since it's all a bit subtle. I think the end result is much easier to reason about though. |
ravikandhadai
approved these changes
Dec 20, 2018
|
@swift-ci Please benchmark |
Build comment file:Performance: -O
Performance: -Osize
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
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.
SILModule::lookUpWitnessTable()had a weird behavior where it would only deserialize a witness table if there was already a declaration for it. As a result, various callers had to ensure that the witness table declaration exists. We would also pre-declare any witness tables referenced from SIL instructions when they were created.None of this was really necessary; instead,
lookUpWitnessTable()should just create the declaration if it was asked to deserialize.This is a nice cleanup in itself, but is mostly meant to support the constexpr work by @marcrasi and @ravikandhadai.