Skip to content

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

Merged
merged 3 commits into from
Nov 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions include/swift/AST/LayoutConstraint.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ class ASTPrinter;
enum class LayoutConstraintKind : uint8_t {
// It is not a known layout constraint.
UnknownLayout,
// It is a layout constraint representing a trivial type of an unknown size.
// It is a layout constraint representing a trivial type of a known size.
TrivialOfExactSize,
// It is a layout constraint representing a trivial type of an unknown size.
// It is a layout constraint representing a trivial type of a size known to
// be no larger than a given size.
TrivialOfAtMostSize,
// It is a layout constraint representing a trivial type of an unknown size.
Trivial,
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ namespace swift {
/// Enable experimental operator designated types feature.
bool EnableOperatorDesignatedTypes = false;

/// Enable SIL type lowering
bool EnableSubstSILFunctionTypesForFunctionValues = false;

/// Enable constraint solver support for experimental
/// operator protocol designator feature.
bool SolverEnableOperatorDesignatedTypes = false;
Expand Down
4 changes: 4 additions & 0 deletions include/swift/Option/FrontendOptions.td
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,10 @@ def enable_experimental_static_assert :
Flag<["-"], "enable-experimental-static-assert">,
HelpText<"Enable experimental #assert">;

def enable_subst_sil_function_types_for_function_values :
Flag<["-"], "enable-subst-sil-function-types-for-function-values">,
HelpText<"Use substituted function types for SIL type lowering of function values">;

def enable_deserialization_recovery :
Flag<["-"], "enable-deserialization-recovery">,
HelpText<"Attempt to recover from missing xrefs (etc) in swiftmodules">;
Expand Down
101 changes: 63 additions & 38 deletions lib/AST/ASTPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4006,54 +4006,79 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
printFunctionExtInfo(T->getExtInfo(),
T->getWitnessMethodConformanceOrInvalid());
printCalleeConvention(T->getCalleeConvention());
if (auto sig = T->getSubstGenericSignature()) {
printGenericSignature(sig,
PrintAST::PrintParams |
PrintAST::PrintRequirements);
Printer << " ";
if (T->isGenericSignatureImplied()) {
Printer << "in ";
}

// If this is a substituted function type, then its generic signature is
// independent of the enclosing context, and defines the parameters active
// in the interface params and results. Unsubstituted types use the existing
// environment, which may be a sil decl's generic environment.
//
// Yeah, this is fiddly. In the end, we probably want all decls to have
// substituted types in terms of a generic signature declared on the decl,
// which would make this logic more uniform.
TypePrinter *sub = this;
Optional<TypePrinter> subBuffer;
PrintOptions subOptions = Options;
if (T->getSubstitutions()) {
subOptions.GenericEnv = nullptr;
subBuffer.emplace(Printer, subOptions);
sub = &*subBuffer;
}

// Capture list used here to ensure we don't print anything using `this`
// printer, but only the sub-Printer.
[T, sub, &subOptions]{
if (auto sig = T->getSubstGenericSignature()) {
sub->printGenericSignature(sig,
PrintAST::PrintParams |
PrintAST::PrintRequirements);
sub->Printer << " ";
if (T->isGenericSignatureImplied()) {
sub->Printer << "in ";
}
}

Printer << "(";
bool first = true;
for (auto param : T->getParameters()) {
Printer.printSeparator(first, ", ");
param.print(Printer, Options);
}
Printer << ") -> ";
sub->Printer << "(";
bool first = true;
for (auto param : T->getParameters()) {
sub->Printer.printSeparator(first, ", ");
param.print(sub->Printer, subOptions);
}
sub->Printer << ") -> ";

unsigned totalResults =
T->getNumYields() + T->getNumResults() + unsigned(T->hasErrorResult());
unsigned totalResults =
T->getNumYields() + T->getNumResults() + unsigned(T->hasErrorResult());

if (totalResults != 1) Printer << "(";
if (totalResults != 1)
sub->Printer << "(";

first = true;
first = true;

for (auto yield : T->getYields()) {
Printer.printSeparator(first, ", ");
Printer << "@yields ";
yield.print(Printer, Options);
}
for (auto yield : T->getYields()) {
sub->Printer.printSeparator(first, ", ");
sub->Printer << "@yields ";
yield.print(sub->Printer, subOptions);
}

for (auto result : T->getResults()) {
Printer.printSeparator(first, ", ");
result.print(Printer, Options);
}
for (auto result : T->getResults()) {
sub->Printer.printSeparator(first, ", ");
result.print(sub->Printer, subOptions);
}

if (T->hasErrorResult()) {
// The error result is implicitly @owned; don't print that.
assert(T->getErrorResult().getConvention() == ResultConvention::Owned);
Printer.printSeparator(first, ", ");
Printer << "@error ";
T->getErrorResult().getInterfaceType().print(Printer, Options);
}
if (T->hasErrorResult()) {
// The error result is implicitly @owned; don't print that.
assert(T->getErrorResult().getConvention() == ResultConvention::Owned);
sub->Printer.printSeparator(first, ", ");
sub->Printer << "@error ";
T->getErrorResult().getInterfaceType().print(sub->Printer, subOptions);
}

if (totalResults != 1) Printer << ")";
if (totalResults != 1)
sub->Printer << ")";
}();

// The substitution types are always in terms of the outer environment.
if (auto substitutions = T->getSubstitutions()) {
Printer << " for ";
Printer << " for";
printSubstitutions(substitutions);
}
}
Expand All @@ -4076,7 +4101,7 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
[&sub, T]{
if (auto sig = T->getLayout()->getGenericSignature()) {
sub.printGenericSignature(sig,
PrintAST::PrintParams | PrintAST::PrintRequirements);
PrintAST::PrintParams | PrintAST::PrintRequirements);
sub.Printer << " ";
}
sub.Printer << "{";
Expand Down
3 changes: 3 additions & 0 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
Opts.EnableExperimentalStaticAssert |=
Args.hasArg(OPT_enable_experimental_static_assert);

Opts.EnableSubstSILFunctionTypesForFunctionValues |=
Args.hasArg(OPT_enable_subst_sil_function_types_for_function_values);

Opts.EnableOperatorDesignatedTypes |=
Args.hasArg(OPT_enable_operator_designated_types);

Expand Down
156 changes: 154 additions & 2 deletions lib/SIL/SILFunctionType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@rjmccall rjmccall Nov 5, 2019

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.

Copy link
Contributor Author

@jckarter jckarter Nov 5, 2019

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.

Copy link
Contributor

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 of OpaqueType but the concrete type is known to be Int. Feels easier to me to just define this away.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@jckarter jckarter Nov 5, 2019

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.

Copy link
Contributor

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.

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;
}
Copy link
Contributor Author

@jckarter jckarter Nov 5, 2019

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.


if (layout)
substRequirements.push_back(
Requirement(RequirementKind::Layout, param, layout));
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 _NativeClass and _SingleRefcounted, which don't seem like useful distinctions from AnyObject at this level. Maybe it'd be better to specifically canonicalize those internal constraints, though.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@jckarter jckarter Nov 5, 2019

Choose a reason for hiding this comment

The 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 _NativeClass instead of AnyObject as their layout constraint when the base class has no ObjC ancestry, and that seems like it could introduce a lot of spurious type mismatches. I can make it so it more selectively generalizes layout constraints, though.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
}

Expand Down Expand Up @@ -3049,7 +3201,7 @@ SILFunctionType::withSubstitutions(SubstitutionMap subs) const {
getExtInfo(), getCoroutineKind(),
getCalleeConvention(),
getParameters(), getYields(), getResults(),
getErrorResult(),
getOptionalErrorResult(),
subs, isGenericSignatureImplied(),
const_cast<SILFunctionType*>(this)->getASTContext());
}
2 changes: 2 additions & 0 deletions lib/SIL/TypeLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1547,6 +1547,8 @@ CanType TypeConverter::computeLoweredRValueType(AbstractionPattern origType,
// - types are turned into their unbridged equivalents, depending
// on the abstract CC
// - ownership conventions are deduced
// - a minimal substituted generic signature is extracted to represent
// possible ABI-compatible substitutions
if (auto substFnType = dyn_cast<AnyFunctionType>(substType)) {
// If the formal type uses a C convention, it is not formally
// abstractable, and it may be subject to implicit bridging.
Expand Down
Loading