Skip to content

[silgen] When performing existential erasure, be sure that we have a … #21099

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[silgen] When performing existential erasure, be sure that we have a … #21099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions lib/SILGen/ManagedValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ ManagedValue ManagedValue::ensurePlusOne(SILGenFunction &SGF,
}

bool ManagedValue::isPlusOne(SILGenFunction &SGF) const {
// If this is an lvalue or in context, the value is not at plus one.
if (isLValue() || isInContext())
return false;

// If this value is SILUndef, return true. SILUndef can always be passed to +1
// APIs.
if (isa<SILUndef>(getValue()))
Expand All @@ -229,6 +233,10 @@ bool ManagedValue::isPlusOne(SILGenFunction &SGF) const {
}

bool ManagedValue::isPlusZero() const {
// If this is an lvalue or in context, the value is not at plus one.
if (isLValue() || isInContext())
return false;

// SILUndef can always be passed to +0 APIs.
if (isa<SILUndef>(getValue()))
return true;
Expand Down
11 changes: 10 additions & 1 deletion lib/SILGen/SILGenConvert.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,12 +613,21 @@ ManagedValue SILGenFunction::emitExistentialErasure(
const TypeLowering &existentialTL,
ArrayRef<ProtocolConformanceRef> conformances,
SGFContext C,
llvm::function_ref<ManagedValue (SGFContext)> F,
function_ref<ManagedValue (SGFContext)> inputF,
bool allowEmbeddedNSError) {
// Mark the needed conformances as used.
for (auto conformance : conformances)
SGM.useConformance(conformance);

// A wrapper around inputF that makes sure we are passed a +1 value if we are
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a hack. Why not convert it to a +1 value where necessary when you actually call F?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually isn't the +1-ness supposed to be encoded in the SGFContext? The original intent of the code was that everything produced a +1 value unless SGFContext.allowPlusZero() was true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slavapestov I don't think it makes sense to do existential erasure on +0 value. You are producing a new value independent of the original value. That says it must be an owned value.

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 am converting it to the +1 when calling F in the caller. This is just the callee can assert that is true in a way that should optimize out in no-asserts build.

I am fine changing the code in the caller to check the SGFContext though instead of doing ensurePlusOne.

// passed a value.
function_ref<ManagedValue(SGFContext)> F = [&](SGFContext ctx) {
auto mv = inputF(ctx);
assert((mv.isInContext() || mv.isLValue() || mv.isPlusOne(*this)) &&
"Expected a plus one value?!");
return mv;
};

// If we're erasing to the 'Error' type, we might be able to get an NSError
// representation more efficiently.
auto &ctx = getASTContext();
Expand Down
2 changes: 1 addition & 1 deletion lib/SILGen/SILGenPoly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ static ManagedValue emitTransformExistential(SILGenFunction &SGF,
[&](SGFContext C) -> ManagedValue {
if (openedArchetype)
return SGF.manageOpaqueValue(state, loc, C);
return input;
return input.ensurePlusOne(SGF, loc);
});

return input;
Expand Down
21 changes: 21 additions & 0 deletions test/SILGen/function_conversion.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,3 +672,24 @@ func dontCrash() {
let userInfo = ["hello": "world"]
let d = [AnyHashable: Any](uniqueKeysWithValues: userInfo.map { ($0.key, $0.value) })
}

@inline(never)
func takesTwoGeneric<T>(_ fn: (T) -> (), _ a: T) -> T {
fn(a)
return a
}

@inline(never)
func callback(_: __owned AnyObject, _: __owned AnyObject) {}

// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$syXlyXlIegxx_19function_conversion5FeralCACIeggg_TR : $@convention(thin) (@guaranteed Feral, @guaranteed Feral, @guaranteed @callee_guaranteed (@owned AnyObject, @owned AnyObject) -> ()) -> () {
// CHECK: bb0([[ARG0:%.*]] : @guaranteed $Feral, [[ARG1:%.*]] : @guaranteed $Feral, [[ARG2:%.*]] :
// CHECK: [[ARG0_COPY:%.*]] = copy_value [[ARG0]]
// CHECK: [[ARG0_EXISTENTIAL:%.*]] = init_existential_ref [[ARG0_COPY]]
// CHECK: [[ARG1_COPY:%.*]] = copy_value [[ARG1]]
// CHECK: [[ARG1_EXISTENTIAL:%.*]] = init_existential_ref [[ARG1_COPY]]
// CHECK: apply {{%.*}}([[ARG0_EXISTENTIAL]], [[ARG1_EXISTENTIAL]]) : $@callee_guaranteed (@owned AnyObject, @owned AnyObject) -> ()
// CHECK: } // end sil function '$syXlyXlIegxx_19function_conversion5FeralCACIeggg_TR'
func test() {
let z = takesTwoGeneric(callback, (Feral(), Feral()))
}