Skip to content

SIL: Optimizer fixes for noncopyable optional chains. #72281

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

Merged
merged 1 commit into from
Mar 13, 2024
Merged

SIL: Optimizer fixes for noncopyable optional chains. #72281

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
3 changes: 3 additions & 0 deletions include/swift/SIL/SILBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,9 @@ class SILBuilder {
assert(!operand->getType().isTrivial(getFunction()) &&
"Should not be passing trivial values to this api. Use instead "
"emitCopyValueOperation");
assert((getModule().getStage() == SILStage::Raw
|| !operand->getType().isMoveOnly())
&& "should not be copying move-only values in canonical SIL");
return insert(new (getModule())
CopyValueInst(getSILDebugLocation(Loc), operand));
}
Expand Down
19 changes: 11 additions & 8 deletions lib/SILGen/SILGenLValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,9 +670,9 @@ static ManagedValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
SILAccessEnforcement enforcement,
std::optional<ActorIsolation> actorIso,
bool noNestedConflict = false) {
return ManagedValue::forLValue(
enterAccessScope(SGF, loc, base, addr.getValue(), typeData,
accessKind, enforcement, actorIso, noNestedConflict));
auto access = enterAccessScope(SGF, loc, base, addr.getValue(), typeData,
accessKind, enforcement, actorIso, noNestedConflict);
return ManagedValue::forLValue(access);
}

// Find the base of the formal access at `address`. If the base requires an
Expand Down Expand Up @@ -4415,11 +4415,9 @@ LValue SILGenLValue::visitBindOptionalExpr(BindOptionalExpr *e,
SILAccessEnforcement::Static,
std::nullopt,
/*no nested conflict*/ true);
} else {
// Take ownership of the base.
optBase = SGF.emitFormalAccessManagedRValueWithCleanup(e,
optBase.getValue());
}
// Take ownership of the base.
optBase = SGF.emitManagedRValueWithCleanup(optBase.getValue());
}
}
// Bind the value, branching to the destination address if there's no
Expand All @@ -4440,11 +4438,16 @@ LValue SILGenLValue::visitBindOptionalExpr(BindOptionalExpr *e,
// Reset the insertion point at the end of hasValueBB so we can
// continue to emit code there.
SGF.B.setInsertionPoint(someBB);

// Project out the payload on the success branch. We can just use a
// naked ValueComponent here; this is effectively a separate l-value.
ManagedValue optPayload =
getPayloadOfOptionalValue(SGF, e, optBase, valueTypeData, accessKind);
// Disable the cleanup if consuming since the consumer should pull straight
// from the address we give them.
if (optBase.getType().isMoveOnly() && isConsumeAccess(baseAccessKind)) {
optPayload = ManagedValue::forLValue(optPayload.forward(SGF));
}
LValue valueLV;
valueLV.add<ValueComponent>(optPayload, std::nullopt, valueTypeData,
/*is rvalue*/optBase.getType().isObject());
Expand Down
10 changes: 8 additions & 2 deletions lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1468,9 +1468,15 @@ SILInstruction *SILCombiner::visitCondBranchInst(CondBranchInst *CBI) {
if (!CBI->getTrueArgs().empty() || !CBI->getFalseArgs().empty())
return nullptr;
auto EnumOperandTy = SEI->getEnumOperand()->getType();
// Type should be loadable
if (!EnumOperandTy.isLoadable(*SEI->getFunction()))
// Type should be loadable and copyable.
// TODO: Generalize to work without copying address-only or noncopyable
// values.
if (!EnumOperandTy.isLoadable(*SEI->getFunction())) {
return nullptr;
}
if (EnumOperandTy.isMoveOnly()) {
return nullptr;
}

// Result of the select_enum should be a boolean.
if (SEI->getType() != CBI->getCondition()->getType())
Expand Down
5 changes: 4 additions & 1 deletion test/SILGen/moveonly_optional_operations_2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func consumingSwitchSubject3(ncp: consuming NCPair) {
// CHECK: [[PAIR_MARK:%.*]] = mark_unresolved_non_copyable_value [assignable_but_not_consumable] [[PAIR_ACCESS]]
// CHECK: [[FIELD:%.*]] = struct_element_addr [[PAIR_MARK]]
// CHECK: [[FIELD_ACCESS:%.*]] = begin_access [deinit] [static] [no_nested_conflict] [[FIELD]]
// CHECK: cond_br {{.*}}, [[SOME_BB:bb[0-9]+]],
// CHECK: cond_br {{.*}}, [[SOME_BB:bb[0-9]+]], [[NONE_BB:bb[0-9]+]]
// CHECK: [[SOME_BB]]:
// CHECK: [[PAYLOAD_ADDR:%.*]] = unchecked_take_enum_data_addr [[FIELD_ACCESS]]
// CHECK: [[PAYLOAD:%.*]] = load [take] [[PAYLOAD_ADDR]]
Expand All @@ -85,6 +85,9 @@ func consumingSwitchSubject3(ncp: consuming NCPair) {
// CHECK-NEXT: end_access [[PAIR_ACCESS]]
// CHECK-NEXT: inject_enum_addr
// CHECK-NEXT: br
// CHECK: [[NONE_BB]]:
// CHECK: destroy_addr [[FIELD_ACCESS]]
// CHECK-NEXT: end_access [[FIELD_ACCESS]]

switch ncp.first?.c3() {
default:
Expand Down