IRGen for conditional conformances #12531
Conversation
|
|
|
@huonw I think it would be easier if you land the first few changes in a separate PR |
Fortunately most of those are just updating tests for a SIL printing/parsing change.
Yes, that's #12430. Even with that one currently lumped in, I think you'll be able to give meaningful input on specifically the last commit, which is the one I know is wrong: 99f1c89 |
lib/IRGen/GenProto.cpp
Outdated
| auto type = req.getFirstType()->getCanonicalType(); | ||
| auto proto = req.getSecondType()->castTo<ProtocolType>()->getDecl(); | ||
|
|
||
| auto reqConformance = subMap.lookupConformance(type, proto); |
There was a problem hiding this comment.
type here could be concrete, which is why I've hacked SubstitutionMap/GenericSignature to support pulling those conformances from the SubstitutionList into the generic signature too.
One alternative I just thought of is to walk through the conditional requirements on the normal conformance, so that the requirements themselves phrased in terms of generic parameters, and then use the substitution map to convert these into concrete types and also this call will work fine without needing hacks elsewhere.
lib/IRGen/GenProto.cpp
Outdated
| auto specialized = conformance; | ||
| if (!conformance.isConcrete() || | ||
| conformance.getConcrete()->getKind() == ProtocolConformanceKind::Normal) | ||
| specialized = conformance.subst(argType, QuerySubstitutionMap{subs}, |
There was a problem hiding this comment.
I'm just making this up/guessing, in an attempt to get a specialized conformance.
|
We figured out the solution offline. Turns out there's a utility method in the devirtualizer that does exactly what we need here. |
huonw
force-pushed
the
conditional-conformance-irgen
branch
from
99f1c89 to
dc28682
Compare
huonw
changed the title
huonw
force-pushed
the
conditional-conformance-irgen
branch
4 times, most recently
from
d8b9760 to
701abfd
Compare
|
Rebased; it's now a more reasonable 37 files changed. |
huonw
force-pushed
the
conditional-conformance-irgen
branch
from
701abfd to
f43c607
Compare
|
@swift-ci please smoke test. |
include/swift/SIL/SILWitnessTable.h
Outdated
| struct ConditionalConformance { | ||
| CanType Requirement; | ||
| ProtocolDecl *Protocol; | ||
| ProtocolConformanceRef Conformance; |
There was a problem hiding this comment.
Isn't Protocol the same as Conformance.getRequirement()?
lib/AST/GenericSignature.cpp
Outdated
| auto canReplacement = replacement->getCanonicalType(); | ||
| if (!canReplacement->isTypeParameter() && | ||
| !canReplacement->is<ArchetypeType>()) { | ||
| result.addConformance(canReplacement, conformances[i]); |
There was a problem hiding this comment.
It surprises me that we need this.
There was a problem hiding this comment.
We don't. Vestigates of a hack that I seemingly didn't fully remove.
| @@ -288,6 +286,45 @@ GenericSignature *ProtocolConformance::getGenericSignature() const { | |||
| llvm_unreachable("Unhandled ProtocolConformanceKind in switch."); | |||
| } | |||
|
|
|||
| SubstitutionMap ProtocolConformance::getSubstitutions(ModuleDecl *M) const { | |||
There was a problem hiding this comment.
There's an existing SpecializedProtocolConformance::getSubstitutionMap(). Should that be generalized to cover this case as well, i.e., by pushing it up to ProtocolConformance::getSubstitutionMap()?
There was a problem hiding this comment.
SpecializedProtocolConformance::getSubstitutionMap doesn't need the ModuleDecl, so I think it makes sense to retain it at least as an implementation detail of this and for internal use in SpecializedProtocolConformance.
| @@ -712,48 +712,6 @@ DevirtualizationResult swift::tryDevirtualizeClassMethod(FullApplySite AI, | |||
| // Witness Method Optimization | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| static SubstitutionMap | |||
| getSubstitutionsForProtocolConformance(ModuleDecl *M, | |||
There was a problem hiding this comment.
Yay, elimination of a duplicate utility method!
| @@ -0,0 +1,153 @@ | |||
| func takes_p1<T: P1>(_: T.Type) {} | |||
There was a problem hiding this comment.
Nah... It's only necessary if you want to run the test.
There was a problem hiding this comment.
Yeah, it's easier to get all tests passing this way. (Thanks.)
huonw
force-pushed
the
conditional-conformance-irgen
branch
from
f43c607 to
ad525fc
Compare
|
Updated with @DougGregor's review. Still need to rebase. |
There was a problem hiding this comment.
Looking good! Needs some more tests:
- SIL parser test for the new witness table syntax
- Executable test building multiple modules to test serialization/deserialization
- Optimizer test for devirtualizing conditional conformance calls, make sure substitution works as expected
lib/IRGen/GenProto.cpp
Outdated
|
|
||
| // Assert that the number of witness tables passed in is the number required | ||
| // for the conditional conformances of this witness table. | ||
| auto failBB = IGF.createBasicBlock("bad_witness_table_count"); |
There was a problem hiding this comment.
I don't think we actually want to check this at runtime, although it might be useful for debugging for now
There was a problem hiding this comment.
Yes, I think it's good for debugging, especially while we bring up the dynamic casting.
lib/IRGen/GenProto.cpp
Outdated
|
|
||
| ConformanceInfo *info; | ||
| // Drill down to the root normal |
| @@ -0,0 +1,84 @@ | |||
| // RUN: %target-run-simple-swift | %FileCheck %s | |||
There was a problem hiding this comment.
Execution tests should go into test/Interpreter/
| @@ -0,0 +1,84 @@ | |||
| // RUN: %target-run-simple-swift | %FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
Please add '// REQUIRES: executable_test'
| @@ -0,0 +1,33 @@ | |||
| // RUN: %empty-directory(%t) | |||
| // The file that's `main` needs to be called that. | |||
| // RUN: cp %s %t/main.swift | |||
There was a problem hiding this comment.
REQUIRES: executable_test, should go into test/Interpreter/.
You can put common files for conditional conformance tests in test/Inputs/
| @@ -94,7 +94,7 @@ sil hidden_external @takingEmptyAndQ : $@convention(thin) <τ_0_0 where τ_0_0 | |||
| // CHECK: [[GENPARAMSADDR:%.*]] = bitcast %swift.type* [[TY]] to %swift.type** | |||
| // CHECK: [[PARAMTYADDR:%.*]] = getelementptr inbounds %swift.type*, %swift.type** [[GENPARAMSADDR]], {{(i64 10|i32 13)}} | |||
| // CHECK: %"BaseProducer<\CF\84_0_1>" = load %swift.type*, %swift.type** [[PARAMTYADDR]] | |||
| // CHECK: [[WITNESS:%.*]] = call i8** @_T023partial_apply_forwarder12BaseProducerVyxGAA1QAAWa(%swift.type* %"BaseProducer<\CF\84_0_1>") | |||
| // CHECK: [[WITNESS:%.*]] = call i8** @_T023partial_apply_forwarder12BaseProducerVyxGAA1QAAWa(%swift.type* %"BaseProducer<\CF\84_0_1>", i8*** null, {{i32|i64}} 0) | |||
There was a problem hiding this comment.
Maybe witness table instantiation for non-conditional conformances should pass undef for the last two arguments -- and the witness table accessor should not read them in the non-conditional case
There was a problem hiding this comment.
I think it would be good to at least retain the 0, so that we can retain the dynamic assertion for now. I will leave fixmes and file a radar about removing/optimizing when we're confident.
| @@ -296,7 +296,13 @@ bb0(%0 : $*ResilientConformingType): | |||
|
|
|||
| // CHECK-LABEL: define{{( protected)?}} i8** @_T019protocol_resilience23ResilientConformingTypeV010resilient_A005OtherC8ProtocolAAWa() | |||
| // CHECK-NEXT: entry: | |||
| // CHECK-NEXT: [[WTABLE:%.*]] = call i8** @swift_rt_swift_getGenericWitnessTable(%swift.generic_witness_table_cache* @_T019protocol_resilience23ResilientConformingTypeV010resilient_A005OtherC8ProtocolAAWG, %swift.type* null, i8** null) | |||
| // CHECK-NEXT: %conditional.tables = alloca %swift.witness_table_slice, align 8 | |||
There was a problem hiding this comment.
See above, this is unnecessary code size bloat in the unconditional conformance case
There was a problem hiding this comment.
I guess we can skip storing the i8*** but the 0 is important, I think, as above.
…ormance tables. Pointers to the witness tables of any conditional conformances are placed into private data, closest to the base pointer, i.e. wtable_ptr[-1] will be the first (if any) conditional conformance witness table. These always need to be provided by the context, and copied in when the witness table is instantiated, making reserving space easy: increment the size of the private data section.
A concrete conformance may involve conditional conformances, which are witness tables that we can access from the original conformance's one. We need to track metadata and be able to follow it in a metadata path.
This requires the witness table accessor function to gain two new parameters: a pointer to an array of witness tables and their count. These are then passed down to the instantiation function which reads them out of the array and writes them into the newly-allocated witness table. We use the count to assert that the number of conditional witness tables passed in is what the protocol conformance expects, which is especially useful while the feature is still experimental: it is a compiler/runtime bug if an incorrect number is passed.
…ents from the self witness table. Conditional requirements shouldn't appear in witness_method's (IR) signature, since they differ from conformance to conformance. PolymorphicConvention just needs to consider these fulfilled by the self witness table, since they can/should be pulled out of there.
…od thunks. These are passed down to the "true" function, and so need to be made available.
…able accessor. When calling an accessor, one has to pull the witness tables for each conditional conformance requirement into a(n appropriately ordered) buffer that is passed to the accessor. This is simple enough, if the appropriate specialization of the relevant conformances are known, which the compiler didn't track deep enough until now.
huonw
force-pushed
the
conditional-conformance-irgen
branch
from
ad525fc to
920cd72
Compare
|
@swift-ci please smoke test. |
huonw
force-pushed
the
conditional-conformance-irgen
branch
from
d250ca4 to
67c9fa3
Compare
|
@swift-ci please smoke test. |
4 similar comments
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test. |
|
@swift-ci please smoke test compiler performance |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test compiler performance |
|
@swift-ci please smoke test Linux platform |
|
@swift-ci Please test source compatibility |
|
But seriously, 🎆 🎉 |
|
@huonw nw This commit breaks the PR bots. Looks like the added IRGen tests are not 32bit save. https://ci.swift.org/job/swift-PR-osx/1283/ |
|
Oh no, pointer alignment :( @huonw if you are not around I might try and take care of this. |
|
Please revert this now and fix it later. PR tests have been broken repeatedly for two days. |
|
@gparker42 I don't think it's sensible to revert them just because of this problem, especially because @huonw put in "PTRSIZE=64" in the test hoping that it's going to work, I'd rather fix it right now. |
|
#12869 , sorry. |
The last commit is me kinda sorta maybe guessing at what substitutions/ProtocolConformances need to be glued together, in a way that works in some cases... but doesn't even compile the standard library (which has no conditional conformances atm).