-
Notifications
You must be signed in to change notification settings - Fork 10.5k
SIL: Type lowering for function values with substituted SIL function types. #28065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4d6392b
0367f8a
27d7117
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1059,11 +1059,163 @@ static CanSILFunctionType getSILFunctionType( | |
.withIsPseudogeneric(pseudogeneric) | ||
.withNoEscape(extInfo.isNoEscape()); | ||
|
||
// Extract the abstract generic calling convention of the function into a | ||
// substituted generic signature for the function type. | ||
// | ||
// This signature only needs to consider the general calling convention, | ||
// so it can reduce away protocol and base class constraints aside from | ||
// `AnyObject`. We wanto similar-shaped generic function types to remain | ||
// canonically equivalent, like `(T, U) -> ()`, `(T, T) -> ()`, | ||
// `(U, T) -> ()` or `(T, T.A) -> ()` when given substitutions that produce | ||
// the same function types, so we also introduce a new generic argument for | ||
// each position where we see a dependent type, and canonicalize the order in | ||
// which we see independent generic arguments. | ||
// | ||
bool impliedSignature = false; | ||
SubstitutionMap substitutions; | ||
if (TC.Context.LangOpts.EnableSubstSILFunctionTypesForFunctionValues | ||
// We don't currently use substituted function types for generic function | ||
// type lowering, though we should for generic methods on classes and | ||
// protocols. | ||
&& !genericSig) { | ||
class SubstFunctionTypeCollector { | ||
public: | ||
TypeConverter &TC; | ||
const bool mapReplacementsOutOfContext; | ||
|
||
SmallVector<GenericTypeParamType *, 4> substGenericParams; | ||
SmallVector<Requirement, 4> substRequirements; | ||
SmallVector<Type, 4> substReplacements; | ||
|
||
SubstFunctionTypeCollector(TypeConverter &TC, | ||
bool mapReplacementsOutOfContext) | ||
: TC(TC), mapReplacementsOutOfContext(mapReplacementsOutOfContext) {} | ||
SubstFunctionTypeCollector(const SubstFunctionTypeCollector &) = delete; | ||
|
||
// TypeSubstitutionFn | ||
Type operator()(SubstitutableType *t) { | ||
ArchetypeType *archetype = dyn_cast<ArchetypeType>(t); | ||
|
||
// We should only substitute contextual archetypes here. | ||
// Opened existential or opaque archetypes are not freely substitutable | ||
// so ought to be left as is. | ||
assert(isa<PrimaryArchetypeType>(archetype->getRoot())); | ||
|
||
if (!archetype) | ||
archetype = TC.getCurGenericContext()->getGenericEnvironment() | ||
->mapTypeIntoContext(t) | ||
->castTo<ArchetypeType>(); | ||
|
||
// Replace every dependent type we see with a fresh type variable in | ||
// the substituted signature. | ||
auto paramIndex = substGenericParams.size(); | ||
auto param = CanGenericTypeParamType::get(0, paramIndex, TC.Context); | ||
|
||
substGenericParams.push_back(param); | ||
Type replacementTy = t; | ||
if (mapReplacementsOutOfContext) { | ||
replacementTy = t->mapTypeOutOfContext(); | ||
} | ||
substReplacements.push_back(replacementTy); | ||
|
||
// Preserve the layout constraint, if any, on the archetype in the | ||
// generic signature, generalizing away some constraints that | ||
// shouldn't affect ABI substitutability. | ||
if (auto layout = archetype->getLayoutConstraint()) { | ||
switch (layout->getKind()) { | ||
// Keep these layout constraints as is. | ||
case LayoutConstraintKind::RefCountedObject: | ||
case LayoutConstraintKind::TrivialOfAtMostSize: | ||
break; | ||
|
||
case LayoutConstraintKind::UnknownLayout: | ||
case LayoutConstraintKind::Trivial: | ||
// These constraints don't really constrain the ABI, so we can | ||
// eliminate them. | ||
layout = LayoutConstraint(); | ||
break; | ||
|
||
// Replace these specific constraints with one of the more general | ||
// constraints above. | ||
case LayoutConstraintKind::NativeClass: | ||
case LayoutConstraintKind::Class: | ||
case LayoutConstraintKind::NativeRefCountedObject: | ||
// These can all be generalized to RefCountedObject. | ||
layout = LayoutConstraint::getLayoutConstraint( | ||
LayoutConstraintKind::RefCountedObject); | ||
break; | ||
|
||
case LayoutConstraintKind::TrivialOfExactSize: | ||
// Generalize to TrivialOfAtMostSize. | ||
layout = LayoutConstraint::getLayoutConstraint( | ||
LayoutConstraintKind::TrivialOfAtMostSize, | ||
layout->getTrivialSizeInBits(), | ||
layout->getAlignmentInBits(), | ||
TC.Context); | ||
break; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjmccall Here's some logic I added to fold up our existing layout constraints into what seemed like the appropriate ABI buckets to me. How's this look? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM. |
||
|
||
if (layout) | ||
substRequirements.push_back( | ||
Requirement(RequirementKind::Layout, param, layout)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we don't really support any other layout constraints in the language right now, but is there a reason for this not to preserve an arbitrary layout constraint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had it that way originally, but we apparently do have distinct layout constraints for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, some of the other layout constraints definitely seem useful to distinguish ABI-wise, and I'm not sure there's harm in just preserving all the original layout constraints. If we specialized an implementation, presumably function arguments would (by default) preserve the original constraints of the function, and it would just be the substitution with the specialized constraint. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main issue I see with that is that base class constraints imply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I hadn't thought of that. Yeah, maybe doing it selectively would make more sense. |
||
|
||
return param; | ||
} | ||
|
||
// LookupConformanceFn | ||
ProtocolConformanceRef operator()(CanType dependentType, | ||
Type conformingReplacementType, | ||
ProtocolDecl *conformedProtocol) { | ||
// We should only substitute with other dependent types, so that an | ||
// abstract conformance ref is sufficient. | ||
assert(conformingReplacementType->isTypeParameter()); | ||
return ProtocolConformanceRef(conformedProtocol); | ||
} | ||
}; | ||
|
||
SubstFunctionTypeCollector collector(TC, | ||
substFnInterfaceType->hasTypeParameter()); | ||
auto contextSig = TC.getCurGenericContext(); | ||
|
||
auto collectSubstitutions = [&](CanType t) -> CanType { | ||
if (t->hasTypeParameter()) { | ||
t = contextSig->getGenericEnvironment() | ||
->mapTypeIntoContext(t) | ||
->getCanonicalType(contextSig); | ||
} | ||
|
||
return CanType(t.subst(collector, collector)); | ||
}; | ||
|
||
for (auto &input : inputs) { | ||
input = input.getWithInterfaceType( | ||
collectSubstitutions(input.getInterfaceType())); | ||
} | ||
for (auto &yield : yields) { | ||
yield = yield.getWithInterfaceType( | ||
collectSubstitutions(yield.getInterfaceType())); | ||
} | ||
for (auto &result : results) { | ||
result = result.getWithInterfaceType( | ||
collectSubstitutions(result.getInterfaceType())); | ||
} | ||
|
||
if (!collector.substGenericParams.empty()) { | ||
genericSig = GenericSignature::get(collector.substGenericParams, | ||
collector.substRequirements) | ||
->getCanonicalSignature(); | ||
substitutions = SubstitutionMap::get(genericSig, | ||
collector.substReplacements, | ||
ArrayRef<ProtocolConformanceRef>()); | ||
impliedSignature = true; | ||
} | ||
} | ||
|
||
return SILFunctionType::get(genericSig, silExtInfo, coroutineKind, | ||
calleeConvention, inputs, yields, | ||
results, errorResult, | ||
SubstitutionMap(), false, | ||
substitutions, impliedSignature, | ||
TC.Context, witnessMethodConformance); | ||
} | ||
|
||
|
@@ -3049,7 +3201,7 @@ SILFunctionType::withSubstitutions(SubstitutionMap subs) const { | |
getExtInfo(), getCoroutineKind(), | ||
getCalleeConvention(), | ||
getParameters(), getYields(), getResults(), | ||
getErrorResult(), | ||
getOptionalErrorResult(), | ||
subs, isGenericSignatureImplied(), | ||
const_cast<SILFunctionType*>(this)->getASTContext()); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is covered correctly in the code, but please clarify in the comment that this also includes opaque types. And that should be tested.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't include opaque types, because
subst
won't substitute them unless explicitly asked to, and I don't think we need to include them because they aren't arbitrarily substitutable by call sites. I can clarify that in the comment, and add a test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, but they are sometimes substituted, right? On the implementation side of an opaque type, we know that
OpaqueType == Int
. If we make opaque-type identity part of the ABI, presumably that means that at some point in SIL on the implementation side we'll need to represent function types where the original type is expressed in terms ofOpaqueType
but the concrete type is known to beInt
. Feels easier to me to just define this away.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opaque type is never formally substituted; we at best do a convert_function or bitcast in places we know the underlying type. Arnold's work on #28044 should more thoroughly handle the relationship between opaque types and their underlying type as part of type lowering, so the difference would only manifest at boundaries between differently-resilient function bodies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the ABI of a specific function has to be consistent, so it matters if the representation would say that they're ABI-different on different sides of the resilience boundary. Concretely, if we're going to give
() -> (@out Opaque)
a different ptrauth discriminator from() -> (@out Int)
, we need to remember that the function signature was actually written as the former, which is what this work is all about; Arnold's work can't just rewrite it on one side of the boundary and in doing so implicitly change the discriminator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, good point. I'll change it to substitute them out.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, doing that correctly in the face of #28044 might require sinking this more deeply into the type lowering logic than I was hoping to start out with, instead of doing it as a post-pass, since the lowered argument and return types would already have the opaque archetypes substituted out of them. If you don't mind, I can do that in a follow up patch after Arnold lands his.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, it just needs to happen before we can make ptrauth discriminator tighter.