From bbc682822a396f95dda65a4fa4234bd0b1a00b73 Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 01:42:58 -0400 Subject: [PATCH 1/7] [NFC] Change the printing of AbstractionPattern to include the sub map --- lib/SIL/IR/AbstractionPattern.cpp | 32 ++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/SIL/IR/AbstractionPattern.cpp b/lib/SIL/IR/AbstractionPattern.cpp index 5ce151a7cf576..92bff9908b4e2 100644 --- a/lib/SIL/IR/AbstractionPattern.cpp +++ b/lib/SIL/IR/AbstractionPattern.cpp @@ -1376,6 +1376,26 @@ void AbstractionPattern::dump() const { llvm::errs() << "\n"; } +static void printGenerics(raw_ostream &out, const AbstractionPattern &pattern) { + if (auto sig = pattern.getGenericSignature()) { + sig->print(out); + } + // It'd be really nice if we could get these interleaved with the types. + if (auto subs = pattern.getGenericSubstitutions()) { + out << "@<"; + bool first = false; + for (auto sub : subs.getReplacementTypes()) { + if (!first) { + out << ","; + } else { + first = true; + } + out << sub; + } + out << ">"; + } +} + void AbstractionPattern::print(raw_ostream &out) const { switch (getKind()) { case Kind::Invalid: @@ -1396,9 +1416,7 @@ void AbstractionPattern::print(raw_ostream &out) const { ? "AP::Type" : getKind() == Kind::Discard ? "AP::Discard" : "<>"); - if (auto sig = getGenericSignature()) { - sig->print(out); - } + printGenerics(out, *this); out << '('; getType().dump(out); out << ')'; @@ -1425,9 +1443,7 @@ void AbstractionPattern::print(raw_ostream &out) const { getKind() == Kind::ObjCCompletionHandlerArgumentsType ? "AP::ObjCCompletionHandlerArgumentsType(" : "AP::CFunctionAsMethodType("); - if (auto sig = getGenericSignature()) { - sig->print(out); - } + printGenerics(out, *this); getType().dump(out); out << ", "; // [TODO: Improve-Clang-type-printing] @@ -1459,9 +1475,7 @@ void AbstractionPattern::print(raw_ostream &out) const { getKind() == Kind::CurriedCXXMethodType ? "AP::CurriedCXXMethodType(" : "AP::PartialCurriedCXXMethodType"); - if (auto sig = getGenericSignature()) { - sig->print(out); - } + printGenerics(out, *this); getType().dump(out); out << ", "; getCXXMethod()->dump(); From f99efc2f9432d2eb56b2a3e3979f8aefce7d2673 Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 01:45:27 -0400 Subject: [PATCH 2/7] Fix unsafeGetSubstFieldType to propagate a substitution map in whichever case it happens to be in. This is a basic fix so that parallel walks on tuples and function types in the substituted type will work . Separately, though, I do not think the places that use this really need to be passed an orig type; this is used for computing type properties, and I am not aware of any reason we should need an orig type to compute type properties. Additionally, the orig types computed by this function are not really correct because of the substitution being done in some cases, so it'd be very nice to rip this all out. I'm not good to look into that right now, though. --- include/swift/SIL/AbstractionPattern.h | 3 ++- lib/SIL/IR/AbstractionPattern.cpp | 9 ++++++--- lib/SIL/IR/TypeLowering.cpp | 11 +++++++---- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/include/swift/SIL/AbstractionPattern.h b/include/swift/SIL/AbstractionPattern.h index e9771df716956..6f31004f6233e 100644 --- a/include/swift/SIL/AbstractionPattern.h +++ b/include/swift/SIL/AbstractionPattern.h @@ -783,7 +783,8 @@ class AbstractionPattern { /// Note that, for most purposes, you should lower a field's type against its /// *unsubstituted* interface type. AbstractionPattern - unsafeGetSubstFieldType(ValueDecl *member, CanType origMemberType) const; + unsafeGetSubstFieldType(ValueDecl *member, CanType origMemberType, + SubstitutionMap subMap) const; private: /// Return an abstraction pattern for the curried type of an diff --git a/lib/SIL/IR/AbstractionPattern.cpp b/lib/SIL/IR/AbstractionPattern.cpp index 92bff9908b4e2..1896258930562 100644 --- a/lib/SIL/IR/AbstractionPattern.cpp +++ b/lib/SIL/IR/AbstractionPattern.cpp @@ -1583,13 +1583,14 @@ bool AbstractionPattern::hasSameBasicTypeStructure(CanType l, CanType r) { AbstractionPattern AbstractionPattern::unsafeGetSubstFieldType(ValueDecl *member, - CanType origMemberInterfaceType) + CanType origMemberInterfaceType, + SubstitutionMap subMap) const { assert(origMemberInterfaceType); if (isTypeParameterOrOpaqueArchetype()) { // Fall back to the generic abstraction pattern for the member. auto sig = member->getDeclContext()->getGenericSignatureOfContext(); - return AbstractionPattern(sig.getCanonicalSignature(), + return AbstractionPattern(subMap, sig.getCanonicalSignature(), origMemberInterfaceType); } @@ -1626,7 +1627,9 @@ const { member, origMemberInterfaceType) ->getReducedType(getGenericSignature()); - return AbstractionPattern(getGenericSignature(), memberTy); + return AbstractionPattern(getGenericSubstitutions(), + getGenericSignature(), + memberTy); } llvm_unreachable("invalid abstraction pattern kind"); } diff --git a/lib/SIL/IR/TypeLowering.cpp b/lib/SIL/IR/TypeLowering.cpp index fe71adc24b2d0..dcb0242bc741b 100644 --- a/lib/SIL/IR/TypeLowering.cpp +++ b/lib/SIL/IR/TypeLowering.cpp @@ -2343,7 +2343,8 @@ namespace { auto sig = field->getDeclContext()->getGenericSignatureOfContext(); auto interfaceTy = field->getInterfaceType()->getReducedType(sig); auto origFieldType = origType.unsafeGetSubstFieldType(field, - interfaceTy); + interfaceTy, + subMap); properties.addSubobject(classifyType(origFieldType, substFieldType, TC, Expansion)); @@ -2422,7 +2423,8 @@ namespace { auto origEltType = origType.unsafeGetSubstFieldType(elt, elt->getArgumentInterfaceType() - ->getReducedType(D->getGenericSignature())); + ->getReducedType(D->getGenericSignature()), + subMap); properties.addSubobject(classifyType(origEltType, substEltType, TC, Expansion)); properties = @@ -2765,7 +2767,8 @@ bool TypeConverter::visitAggregateLeaves( auto interfaceTy = structField->getInterfaceType()->getReducedType(sig); auto origFieldType = - origTy.unsafeGetSubstFieldType(structField, interfaceTy); + origTy.unsafeGetSubstFieldType(structField, interfaceTy, + subMap); insertIntoWorklist(substFieldTy, origFieldType, structField, llvm::None); } @@ -2782,7 +2785,7 @@ bool TypeConverter::visitAggregateLeaves( ->getCanonicalType(); auto origElementTy = origTy.unsafeGetSubstFieldType( element, element->getArgumentInterfaceType()->getReducedType( - decl->getGenericSignature())); + decl->getGenericSignature()), subMap); insertIntoWorklist(substElementType, origElementTy, element, llvm::None); From 8ff61d6a54e9ed7905dbba238db58d9ed8fbf540 Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 01:53:35 -0400 Subject: [PATCH 3/7] When we are performing SIL substitution, and we reach a type that needs to be lowered, use an opaque abstraction pattern. As I argue in the comment, we know that the orig type is now either an opaque type or a type with high-level structure that is invariant to lowering. Substitution will not change the latter property, and an opaque abstraction pattern is correct for the former. Attempting to create a "truer" abstraction pattern that preserves more structure from the orig type is both pointless and problematic. The substitutions we just did may have replaced pack references with non-pack types if there are active expansions in progress; this cannot be easily explained in terms of substitutions. (In theory, we could pass a more opaque concept of substitutions through AbstractionPattern, which might help with this. That would also make it harder to catch bugs with signature mismatches, though.) --- lib/SIL/IR/SILFunctionType.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/SIL/IR/SILFunctionType.cpp b/lib/SIL/IR/SILFunctionType.cpp index 4ed98fb82dcf6..28f21ac4f49da 100644 --- a/lib/SIL/IR/SILFunctionType.cpp +++ b/lib/SIL/IR/SILFunctionType.cpp @@ -4748,7 +4748,17 @@ class SILTypeSubstituter : return origType; } - AbstractionPattern abstraction(Sig, origType); + // We've looked through all the top-level structure in the orig + // type that's affected by type lowering. If substitution has + // given us a type with top-level structure that's affected by + // type lowering, it must be because the orig type was a type + // variable of some sort, and we should lower using an opaque + // abstraction pattern. If substitution hasn't given us such a + // type, it doesn't matter what abstraction pattern we use, + // lowering will just come back with substType. So we can just + // use an opaque abstraction pattern here and not put any effort + // into computing a more "honest" abstraction pattern. + AbstractionPattern abstraction = AbstractionPattern::getOpaque(); return TC.getLoweredRValueType(typeExpansionContext, abstraction, substType); } From a0eeabc88656abaad1c5ae559e6155b8f36e4a8f Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 02:04:05 -0400 Subject: [PATCH 4/7] When computing the field type of a SILType, add substitutions to the orig type. --- lib/SIL/IR/SILType.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/lib/SIL/IR/SILType.cpp b/lib/SIL/IR/SILType.cpp index e4ee158320678..a0cbcdb204453 100644 --- a/lib/SIL/IR/SILType.cpp +++ b/lib/SIL/IR/SILType.cpp @@ -301,9 +301,30 @@ bool SILType::canRefCast(SILType operTy, SILType resultTy, SILModule &M) { && toTy.isHeapObjectReferenceType(); } +static bool needsFieldSubstitutions(const AbstractionPattern &origType) { + if (origType.isTypeParameter()) return false; + auto type = origType.getType(); + if (!type->hasTypeParameter()) return false; + return type.findIf([](CanType type) { + return isa(type); + }); +} + +static void addFieldSubstitutionsIfNeeded(TypeConverter &TC, SILType ty, + ValueDecl *field, + AbstractionPattern &origType) { + if (needsFieldSubstitutions(origType)) { + auto subMap = ty.getASTType()->getContextSubstitutionMap( + &TC.M, field->getDeclContext()); + origType = origType.withSubstitutions(subMap); + } +} + SILType SILType::getFieldType(VarDecl *field, TypeConverter &TC, TypeExpansionContext context) const { AbstractionPattern origFieldTy = TC.getAbstractionPattern(field); + addFieldSubstitutionsIfNeeded(TC, *this, field, origFieldTy); + CanType substFieldTy; if (field->hasClangNode()) { substFieldTy = origFieldTy.getType(); @@ -372,6 +393,9 @@ SILType SILType::getEnumElementType(EnumElementDecl *elt, TypeConverter &TC, getCategory()); } + auto origEltType = TC.getAbstractionPattern(elt); + addFieldSubstitutionsIfNeeded(TC, *this, elt, origEltType); + auto substEltTy = getASTType()->getTypeOfMember( &TC.M, elt, elt->getArgumentInterfaceType()); auto loweredTy = TC.getLoweredRValueType( From 9fbecde9a0f9b726945414c322fb6a94ad3a9996 Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 02:05:21 -0400 Subject: [PATCH 5/7] Do proper parallel walks of orig+subst types when computing type properties for struct and enum types. --- lib/SIL/IR/TypeLowering.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/SIL/IR/TypeLowering.cpp b/lib/SIL/IR/TypeLowering.cpp index dcb0242bc741b..26bc61f04e11b 100644 --- a/lib/SIL/IR/TypeLowering.cpp +++ b/lib/SIL/IR/TypeLowering.cpp @@ -749,11 +749,12 @@ namespace { RetTy visitTupleType(CanTupleType type, AbstractionPattern origType, IsTypeExpansionSensitive_t isSensitive) { RecursiveProperties props; - for (unsigned i = 0, e = type->getNumElements(); i < e; ++i) { - props.addSubobject(classifyType(origType.getTupleElementType(i), - type.getElementType(i), - TC, Expansion)); - } + origType.forEachExpandedTupleElement(type, + [&](AbstractionPattern origEltType, CanType substEltType, + const TupleTypeElt &elt) { + props.addSubobject( + classifyType(origEltType, substEltType, TC, Expansion)); + }); props = mergeIsTypeExpansionSensitive(isSensitive, props); return asImpl().handleAggregateByProperties(type, props); } @@ -2250,12 +2251,12 @@ namespace { AbstractionPattern origType, IsTypeExpansionSensitive_t isSensitive) { RecursiveProperties properties; - for (unsigned i = 0, e = tupleType->getNumElements(); i < e; ++i) { - auto eltType = tupleType.getElementType(i); - auto origEltType = origType.getTupleElementType(i); - auto &lowering = TC.getTypeLowering(origEltType, eltType, Expansion); - properties.addSubobject(lowering.getRecursiveProperties()); - } + origType.forEachExpandedTupleElement(tupleType, + [&](AbstractionPattern origEltType, CanType substEltType, + const TupleTypeElt &elt) { + properties.addSubobject( + classifyType(origEltType, substEltType, TC, Expansion)); + }); properties = mergeIsTypeExpansionSensitive(isSensitive, properties); return handleAggregateByProperties(tupleType, From d18b914fc3910d44d2ab4d1aeba7f9fc9f8806ca Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 02:06:15 -0400 Subject: [PATCH 6/7] Do a proper orig+subst walk of tuple expression elements in call argument emission. --- lib/SILGen/SILGenApply.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index 78eda39d4ce3d..5fc3d2caa5ed5 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -3389,10 +3389,23 @@ class ArgEmitter { // If the source expression is a tuple literal, we can break it // up directly. if (auto tuple = dyn_cast(e)) { - for (auto i : indices(tuple->getElements())) { - emit(tuple->getElement(i), - origParamType.getTupleElementType(i)); - } + auto substTupleType = + cast(e->getType()->getCanonicalType()); + origParamType.forEachTupleElement(substTupleType, + [&](unsigned origEltIndex, unsigned substEltIndex, + AbstractionPattern origEltType, CanType substEltType) { + emit(tuple->getElement(substEltIndex), origEltType); + }, + [&](unsigned origEltIndex, unsigned substEltIndex, + AbstractionPattern origExpansionType, + CanTupleEltTypeArrayRef substEltTypes) { + SmallVector eltArgs; + eltArgs.reserve(substEltTypes.size()); + for (auto i : range(substEltIndex, substEltTypes.size())) { + eltArgs.emplace_back(tuple->getElement(i)); + } + emitPackArg(eltArgs, origExpansionType); + }); return; } From c032cc54087cdd7274a90b8020a88f29510c180b Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 22 Mar 2023 02:08:03 -0400 Subject: [PATCH 7/7] Add a test case for the work in this PR. My original test case here used a memberwise initializer, but those use their own logic for binding and forward parameters which will need to be updated separately. --- test/SILGen/variadic-generic-tuples.swift | 49 +++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/SILGen/variadic-generic-tuples.swift b/test/SILGen/variadic-generic-tuples.swift index a0fe0fad7bb03..fbcfbe6ec51e7 100644 --- a/test/SILGen/variadic-generic-tuples.swift +++ b/test/SILGen/variadic-generic-tuples.swift @@ -204,3 +204,52 @@ func projectTupleElements(_ value: repeat Wrapper) { let tuple = (repeat (each value).value) } + +func takesVariadicTuple(tuple: (repeat each T)) {} + +// CHECK-LABEL: sil{{.*}} @$s4main28testConcreteVariadicTupleArg1i1sySi_SStF : +// CHECK: [[PACK:%.*]] = alloc_pack $Pack{Int, String} +// CHECK-NEXT: [[I_COPY:%.*]] = alloc_stack $Int +// CHECK-NEXT: store %0 to [trivial] [[I_COPY]] : $*Int +// CHECK-NEXT: [[I_INDEX:%.*]] = scalar_pack_index 0 of $Pack{Int, String} +// CHECK-NEXT: pack_element_set [[I_COPY]] : $*Int into [[I_INDEX]] of [[PACK]] : +// CHECK-NEXT: [[S_COPY:%.*]] = alloc_stack $String +// CHECK-NEXT: [[T0:%.*]] = copy_value %1 : $String +// CHECK-NEXT: store [[T0]] to [init] [[S_COPY]] : $*String +// CHECK-NEXT: [[S_INDEX:%.*]] = scalar_pack_index 1 of $Pack{Int, String} +// CHECK-NEXT: pack_element_set [[S_COPY]] : $*String into [[S_INDEX]] of [[PACK]] : +// CHECK-NEXT: // function_ref +// CHECK-NEXT: [[FN:%.*]] = function_ref @$s4main18takesVariadicTuple5tupleyxxQp_t_tRvzlF : $@convention(thin) (@pack_guaranteed Pack{repeat each τ_0_0}) -> () +// CHECK-NEXT: apply [[FN]]([[PACK]]) +// CHECK-NEXT: destroy_addr [[S_COPY]] : +// CHECK-NEXT: dealloc_stack [[S_COPY]] : +// CHECK-NEXT: dealloc_stack [[I_COPY]] : +// CHECK-NEXT: dealloc_pack [[PACK]] : +func testConcreteVariadicTupleArg(i: Int, s: String) { + takesVariadicTuple(tuple: (i, s)) +} + +struct TupleHolder { + var content: (repeat each T) + + // Suppress the memberwise initializer + init(values: repeat each T) { + content = (repeat each values) + } +} + +// CHECK-LABEL: sil{{.*}} @$s4main31takesConcreteTupleHolderFactory7factoryyAA0dE0VySi_SSQPGyXE_tF : +// CHECK-SAME: $@convention(thin) (@guaranteed @noescape @callee_guaranteed () -> @owned TupleHolder) -> () +// CHECK: [[T0:%.*]] = copy_value %0 : +// CHECK: [[T1:%.*]] = begin_borrow [[T0]] +// CHECK: [[RESULT:%.*]] = apply [[T1]]() : +// CHECK: destroy_value [[RESULT]] +func takesConcreteTupleHolderFactory(factory: () -> TupleHolder) { + let holder = factory() +} + +/* We still crash with memberwise initializers +func generateConcreteMemberTuple() -> TupleHolder { + return HasMemberTuple(content: (0, "hello")) +} + */