[sil-serializer] Do not use RPOT order for serializing SIL basic blocks #3546
Conversation
|
This is a re-worked version of SIL basic block serialization bits from #3456. It does not silently changes the order of BBs during the serialization anymore. |
|
@swift-ci Please test |
|
@gottesmm This is the alternative solution I was talking about. Do you like it better? :-) |
Serialize SIL basic blocks in the RPOT order to make sure that instructions defining open archetypes are serialized before instructions using those opened archetypes.
|
Essentially, the patch does the following:
|
| Type SILDeserializer::getType(SILBuilder &Builder, ModuleFile *MF, | ||
| serialization::TypeID TID) { | ||
| auto Ty = MF->getType(TID); | ||
| if (Ty && Ty->hasOpenedExistential()) { |
There was a problem hiding this comment.
Why not invert this condition and early exit. It eliminates a level of indentation.
swiftix
force-pushed
the
cse-of-open-existentials
branch
from
8690c4c to
e4dd8d0
Compare
| llvm_unreachable( | ||
| "All forward definitions of opened archetypes should be resolved"); | ||
| } | ||
| } |
There was a problem hiding this comment.
What about:
if (std::none_of(OpenedArchetypesTracker.getOpenedArchetypeDefs()),
[](KVTy &KV) -> Bool { return !KV.getSecond() || isa(KV.getSecond())) {
llvm_unreachable("All forward definitions of opened archetypes should be resolved");
}
There was a problem hiding this comment.
Closures are cool, but I'm not sure it makes the code more understandable in this case.
There was a problem hiding this comment.
Then at least move it into a helper function. In general, these sorts of predicate loops are not LLVM style:
http://llvm.org/docs/CodingStandards.html#turn-predicate-loops-into-predicate-functions
swiftix
force-pushed
the
cse-of-open-existentials
branch
3 times, most recently
from
b5261ad to
1be4db0
Compare
|
@swift-ci Please smoke test |
My earlier patch started serializing SIL basic blocks using the RPOT order. While it works, changing the existing order of BBs during the serialization may be very surprising for users. After all, serialization is not supposed to transform the code. Therefore, this patch follows a different approach. It uses the existing order of BBs during the serialization. When it deserializes SIL and detects a use of an opened archetype before its definition, it basically introduced a forward definition of this opened archetype. Later on, when the actual definition of the opened archetype is found, it replaces the forward definition. There is a correctness check at the end of a SIL function deserialization, which verifies that there are no forward definitions of opened archetypes left unresoved.
swiftix
force-pushed
the
cse-of-open-existentials
branch
from
1be4db0 to
2f9ee20
Compare
|
@swift-ci Please smoke test |
|
Looks beautiful! |
|
The OS X smoke test failure seems to be unrelated. I cannot reproduce it locally. |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
My earlier patch started serializing SIL basic blocks using the RPOT order. While it works, changing the existing order of BBs during the serialization may be very surprising for users. After all, serialization is not supposed to transform the code.
Therefore, this patch follows a different approach. It uses the existing order of BBs during the serialization. When it deserializes SIL and detects a use of an opened archetype before its definition, it basically introduced a forward definition of this opened archetype. Later on, when the actual definition of the opened archetype is found, it replaces the forward definition. There is a correctness check at the end of a SIL function deserialization, which verifies that there are no forward definitions of opened archetypes left unresoved.