Skip to content

Commit a571357

Browse files
committed
[move-only] Change noncopyable lets to be emitted as boxes like vars.
Some notes: 1. This ensures that if we capture them, we just capture the box by reference. 2. We are still using the old incorrect semantics for captures. I am doing this so I can bring this up in separate easy to understand patches all of which pass all of the moveonly tests. 3. Most of the test edits are due to small differences in error messages in between the object and address checker. 4. I had to add a little support to the move only address checker for a small pattern that doesn't occur with vars but do es occur for lets when we codegen like this, specifically around enums. The pattern is we perform a load_borrow and then copy_value and then use the result of the copy_value. Rather than fight SILGen pattern I introduced a small canonicalization into the address checker which transforms that pattern into a load [copy] + begin_borrow to restore the codegen to a pattern the checker expects. 5. I left noimplicitcopy alone for now. But we should come back around and fix it in a similar way. I just did not have time to do so.
1 parent f4e1b2a commit a571357

13 files changed

+419
-118
lines changed

lib/SILGen/SILGenDecl.cpp

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -493,9 +493,13 @@ class LetValueInitialization : public Initialization {
493493
isUninitialized = true;
494494
} else {
495495
// If this is a let with an initializer or bound value, we only need a
496-
// buffer if the type is address only.
496+
// buffer if the type is address only or is noncopyable.
497+
//
498+
// For noncopyable types, we always need to box them and eagerly
499+
// reproject.
497500
needsTemporaryBuffer =
498-
lowering->isAddressOnly() && SGF.silConv.useLoweredAddresses();
501+
(lowering->isAddressOnly() && SGF.silConv.useLoweredAddresses()) ||
502+
lowering->getLoweredType().isPureMoveOnly();
499503
}
500504

501505
// Make sure that we have a non-address only type when binding a
@@ -642,8 +646,7 @@ class LetValueInitialization : public Initialization {
642646
// We do this before the begin_borrow "normal" path below since move only
643647
// types do not have no implicit copy attr on them.
644648
if (value->getOwnershipKind() == OwnershipKind::Owned &&
645-
value->getType().isMoveOnly() &&
646-
!value->getType().isMoveOnlyWrapped()) {
649+
value->getType().isPureMoveOnly()) {
647650
value = SGF.B.createMoveValue(PrologueLoc, value, true /*isLexical*/);
648651
return SGF.B.createMarkMustCheckInst(
649652
PrologueLoc, value,
@@ -1261,10 +1264,10 @@ SILGenFunction::emitInitializationForVarDecl(VarDecl *vd, bool forceImmutable) {
12611264

12621265
assert(!isa<InOutType>(varType) && "local variables should never be inout");
12631266

1264-
// If this is a 'let' initialization for a non-global, set up a
1265-
// let binding, which stores the initialization value into VarLocs directly.
1267+
// If this is a 'let' initialization for a copyable non-global, set up a let
1268+
// binding, which stores the initialization value into VarLocs directly.
12661269
if (forceImmutable && vd->getDeclContext()->isLocalContext() &&
1267-
!isa<ReferenceStorageType>(varType))
1270+
!isa<ReferenceStorageType>(varType) && !varType->isPureMoveOnly())
12681271
return InitializationPtr(new LetValueInitialization(vd, *this));
12691272

12701273
// If the variable has no initial value, emit a mark_uninitialized instruction

lib/SILGen/SILGenFunction.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ void SILGenFunction::emitCaptures(SILLocation loc,
383383
case CaptureKind::Constant: {
384384
// let declarations.
385385
auto &tl = getTypeLowering(valueType);
386-
SILValue Val = Entry.value;
386+
SILValue Val = Entry.getValueOrBoxedValue(*this);
387387
bool eliminateMoveOnlyWrapper =
388388
Val->getType().isMoveOnlyWrapped() &&
389389
!vd->getInterfaceType()->is<SILMoveOnlyWrappedType>();

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

Lines changed: 156 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,8 +314,14 @@ static bool memInstMustConsume(Operand *memOper) {
314314
return applySite.getArgumentOperandConvention(*memOper).isOwnedConvention();
315315
}
316316
case SILInstructionKind::PartialApplyInst: {
317-
// If we are on the stack, we do not consume. Otherwise, we do.
318-
return !cast<PartialApplyInst>(memInst)->isOnStack();
317+
// If we are on the stack or have an inout convention, we do not
318+
// consume. Otherwise, we do.
319+
auto *pai = cast<PartialApplyInst>(memInst);
320+
if (pai->isOnStack())
321+
return false;
322+
ApplySite applySite(pai);
323+
auto convention = applySite.getArgumentConvention(*memOper);
324+
return convention.isInoutConvention();
319325
}
320326
case SILInstructionKind::DestroyAddrInst:
321327
return true;
@@ -987,6 +993,141 @@ struct MoveOnlyAddressCheckerPImpl {
987993

988994
} // namespace
989995

996+
//===----------------------------------------------------------------------===//
997+
// MARK: CopiedLoadBorrowElimination
998+
//===----------------------------------------------------------------------===//
999+
1000+
namespace {
1001+
1002+
/// An early transform that we run to convert any load_borrow that are copied
1003+
/// directly or that have any subelement that is copied to a load [copy]. This
1004+
/// lets the rest of the optimization handle these as appropriate.
1005+
struct CopiedLoadBorrowEliminationVisitor : public AccessUseVisitor {
1006+
SILFunction *fn;
1007+
StackList<LoadBorrowInst *> targets;
1008+
1009+
CopiedLoadBorrowEliminationVisitor(SILFunction *fn)
1010+
: AccessUseVisitor(AccessUseType::Overlapping,
1011+
NestedAccessType::IgnoreAccessBegin),
1012+
fn(fn), targets(fn) {}
1013+
1014+
bool visitUse(Operand *op, AccessUseType useTy) override {
1015+
LLVM_DEBUG(llvm::dbgs() << "CopiedLBElim. Visiting: " << *op->getUser());
1016+
auto *lbi = dyn_cast<LoadBorrowInst>(op->getUser());
1017+
if (!lbi)
1018+
return true;
1019+
1020+
LLVM_DEBUG(llvm::dbgs() << "Found load_borrow: " << *lbi);
1021+
1022+
StackList<Operand *> useWorklist(lbi->getFunction());
1023+
for (auto *use : lbi->getUses())
1024+
useWorklist.push_back(use);
1025+
1026+
bool shouldConvertToLoadCopy = false;
1027+
while (!useWorklist.empty()) {
1028+
auto *nextUse = useWorklist.pop_back_val();
1029+
switch (nextUse->getOperandOwnership()) {
1030+
case OperandOwnership::NonUse:
1031+
case OperandOwnership::ForwardingUnowned:
1032+
case OperandOwnership::PointerEscape:
1033+
continue;
1034+
1035+
// These might be uses that we need to perform a destructure or insert
1036+
// struct_extracts for.
1037+
case OperandOwnership::TrivialUse:
1038+
case OperandOwnership::InstantaneousUse:
1039+
case OperandOwnership::UnownedInstantaneousUse:
1040+
case OperandOwnership::InteriorPointer:
1041+
case OperandOwnership::BitwiseEscape: {
1042+
// Look through copy_value of a move only value. We treat copy_value of
1043+
// copyable values as normal uses.
1044+
if (auto *cvi = dyn_cast<CopyValueInst>(nextUse->getUser())) {
1045+
if (cvi->getOperand()->getType().isMoveOnly()) {
1046+
shouldConvertToLoadCopy = true;
1047+
break;
1048+
}
1049+
}
1050+
continue;
1051+
}
1052+
1053+
case OperandOwnership::ForwardingConsume:
1054+
case OperandOwnership::DestroyingConsume:
1055+
// We can only hit this if our load_borrow was copied.
1056+
llvm_unreachable("We should never hit this");
1057+
1058+
case OperandOwnership::GuaranteedForwarding:
1059+
// If we have a switch_enum, we always need to convert it to a load
1060+
// [copy] since we need to destructure through it.
1061+
shouldConvertToLoadCopy |= isa<SwitchEnumInst>(nextUse->getUser());
1062+
1063+
ForwardingOperand(nextUse).visitForwardedValues([&](SILValue value) {
1064+
for (auto *use : value->getUses()) {
1065+
useWorklist.push_back(use);
1066+
}
1067+
return true;
1068+
});
1069+
continue;
1070+
1071+
case OperandOwnership::Borrow:
1072+
LLVM_DEBUG(llvm::dbgs() << " Found recursive borrow!\n");
1073+
// Look through borrows.
1074+
for (auto value : nextUse->getUser()->getResults()) {
1075+
for (auto *use : value->getUses()) {
1076+
useWorklist.push_back(use);
1077+
}
1078+
}
1079+
continue;
1080+
case OperandOwnership::EndBorrow:
1081+
LLVM_DEBUG(llvm::dbgs() << " Found end borrow!\n");
1082+
continue;
1083+
case OperandOwnership::Reborrow:
1084+
llvm_unreachable("Unsupported for now?!");
1085+
}
1086+
1087+
if (shouldConvertToLoadCopy)
1088+
break;
1089+
}
1090+
1091+
LLVM_DEBUG(llvm::dbgs()
1092+
<< "Load Borrow was copied: "
1093+
<< (shouldConvertToLoadCopy ? "true" : "false") << '\n');
1094+
if (!shouldConvertToLoadCopy)
1095+
return true;
1096+
1097+
targets.push_back(lbi);
1098+
return true;
1099+
}
1100+
1101+
void process() {
1102+
if (targets.empty())
1103+
return;
1104+
1105+
while (!targets.empty()) {
1106+
auto *lbi = targets.pop_back_val();
1107+
SILBuilderWithScope builder(lbi);
1108+
SILValue li = builder.emitLoadValueOperation(
1109+
lbi->getLoc(), lbi->getOperand(), LoadOwnershipQualifier::Copy);
1110+
SILValue borrow = builder.createBeginBorrow(lbi->getLoc(), li);
1111+
1112+
for (auto *ebi : lbi->getEndBorrows()) {
1113+
auto *next = ebi->getNextInstruction();
1114+
SILBuilderWithScope builder(next);
1115+
auto loc = RegularLocation::getAutoGeneratedLocation();
1116+
builder.emitDestroyValueOperation(loc, li);
1117+
}
1118+
1119+
lbi->replaceAllUsesWith(borrow);
1120+
lbi->eraseFromParent();
1121+
}
1122+
1123+
LLVM_DEBUG(llvm::dbgs() << "After Load Borrow Elim. Func Dump Start! ";
1124+
fn->print(llvm::dbgs()));
1125+
LLVM_DEBUG(llvm::dbgs() << "After Load Borrow Elim. Func Dump End!\n");
1126+
}
1127+
};
1128+
1129+
} // namespace
1130+
9901131
//===----------------------------------------------------------------------===//
9911132
// MARK: GatherLexicalLifetimeUseVisitor
9921133
//===----------------------------------------------------------------------===//
@@ -1892,6 +2033,19 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
18922033
return false;
18932034
}
18942035

2036+
// Before we do anything, convert any load_borrow + copy_value into load
2037+
// [copy] + begin_borrow for further processing.
2038+
{
2039+
CopiedLoadBorrowEliminationVisitor copiedLoadBorrowEliminator(fn);
2040+
if (!visitAccessPathBaseUses(copiedLoadBorrowEliminator, accessPathWithBase,
2041+
fn)) {
2042+
LLVM_DEBUG(llvm::dbgs() << "Failed copied load borrow eliminator visit: "
2043+
<< *markedAddress);
2044+
return false;
2045+
}
2046+
copiedLoadBorrowEliminator.process();
2047+
}
2048+
18952049
// Then gather all uses of our address by walking from def->uses. We use this
18962050
// to categorize the uses of this address into their ownership behavior (e.x.:
18972051
// init, reinit, take, destroy, etc.).

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1574,7 +1574,13 @@ static bool gatherBorrows(SILValue rootValue,
15741574
if (!use->get()->getType().isMoveOnly())
15751575
continue;
15761576

1577+
// Do not look through apply sites.
1578+
if (ApplySite::isa(use->getUser()))
1579+
continue;
1580+
15771581
// Search through forwarding consumes.
1582+
//
1583+
// TODO: Can this just not return a forwarded value for ApplySites?
15781584
ForwardingOperand(use).visitForwardedValues([&](SILValue value) -> bool {
15791585
for (auto *use : value->getUses())
15801586
worklist.push_back(use);

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,9 @@ static void getVariableNameForValue(SILValue value2,
106106
// Otherwise, try to see if we have a single value instruction we can look
107107
// through.
108108
if (isa<BeginBorrowInst>(searchValue) || isa<LoadInst>(searchValue) ||
109-
isa<BeginAccessInst>(searchValue) || isa<MarkMustCheckInst>(searchValue) ||
110-
isa<ProjectBoxInst>(searchValue)) {
109+
isa<LoadBorrowInst>(searchValue) || isa<BeginAccessInst>(searchValue) ||
110+
isa<MarkMustCheckInst>(searchValue) ||
111+
isa<ProjectBoxInst>(searchValue) || isa<CopyValueInst>(searchValue)) {
111112
searchValue = cast<SingleValueInstruction>(searchValue)->getOperand(0);
112113
continue;
113114
}

test/SILGen/moveonly.swift

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -168,27 +168,35 @@ extension NonTrivialEnum {
168168
///////////////////////////////
169169

170170
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly27blackHoleLetInitialization1yyF : $@convention(thin) () -> () {
171+
// CHECK: [[BOX:%.*]] = alloc_box
172+
// CHECK: [[BORROWED_BOX:%.*]] = begin_borrow [lexical] [[BOX]]
171173
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly2FDVACycfC :
172174
// CHECK: [[X:%.*]] = apply [[FN]](
173-
// CHECK: [[X_MV_LEXICAL:%.*]] = move_value [lexical] [[X]]
174-
// CHECK: [[X_MV_ONLY:%.*]] = mark_must_check [consumable_and_assignable] [[X_MV_LEXICAL]]
175-
// CHECK: [[X_MV_ONLY_BORROW:%.*]] = begin_borrow [[X_MV_ONLY]]
176-
// CHECK: [[X_MV_ONLY_COPY:%.*]] = copy_value [[X_MV_ONLY_BORROW]]
177-
// CHECK: [[X_MV_ONLY_CONSUME:%.*]] = move_value [[X_MV_ONLY_COPY]]
175+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROWED_BOX]]
176+
// CHECK: store [[X]] to [init] [[PROJECT]]
177+
//
178+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROWED_BOX]]
179+
// CHECK: [[MARKED:%.*]] = mark_must_check [assignable_but_not_consumable] [[PROJECT]]
180+
// CHECK: [[VALUE:%.*]] = load [copy] [[MARKED]]
181+
// CHECK: move_value [[VALUE]]
178182
// CHECK: } // end sil function '$s8moveonly27blackHoleLetInitialization1yyF'
179183
func blackHoleLetInitialization1() {
180184
let x = FD()
181185
let _ = x
182186
}
183187

184188
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly27blackHoleLetInitialization2yyF : $@convention(thin) () -> () {
189+
// CHECK: [[BOX:%.*]] = alloc_box
190+
// CHECK: [[BORROWED_BOX:%.*]] = begin_borrow [lexical] [[BOX]]
185191
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly2FDVACycfC :
186192
// CHECK: [[X:%.*]] = apply [[FN]](
187-
// CHECK: [[X_MV_LEXICAL:%.*]] = move_value [lexical] [[X]]
188-
// CHECK: [[X_MV_ONLY:%.*]] = mark_must_check [consumable_and_assignable] [[X_MV_LEXICAL]]
189-
// CHECK: [[X_MV_ONLY_BORROW:%.*]] = begin_borrow [[X_MV_ONLY]]
190-
// CHECK: [[X_MV_ONLY_COPY:%.*]] = copy_value [[X_MV_ONLY_BORROW]]
191-
// CHECK: [[X_MV_ONLY_CONSUME:%.*]] = move_value [[X_MV_ONLY_COPY]]
193+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROWED_BOX]]
194+
// CHECK: store [[X]] to [init] [[PROJECT]]
195+
//
196+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROWED_BOX]]
197+
// CHECK: [[MARKED:%.*]] = mark_must_check [assignable_but_not_consumable] [[PROJECT]]
198+
// CHECK: [[VALUE:%.*]] = load [copy] [[MARKED]]
199+
// CHECK: move_value [[VALUE]]
192200
// CHECK: } // end sil function '$s8moveonly27blackHoleLetInitialization2yyF'
193201
func blackHoleLetInitialization2() {
194202
let x = FD()
@@ -274,8 +282,15 @@ func blackHoleVarInitialization3() {
274282
////////////////////////////////
275283

276284
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly24borrowObjectFunctionCallyyF : $@convention(thin) () -> () {
277-
// CHECK: [[CLS:%.*]] = mark_must_check [consumable_and_assignable]
278-
// CHECK: [[BORROW:%.*]] = begin_borrow [[CLS]]
285+
// CHECK: [[BOX:%.*]] = alloc_box
286+
// CHECK: [[BORROW_BOX:%.*]] = begin_borrow [lexical] [[BOX]]
287+
//
288+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROW_BOX]]
289+
// CHECK: store {{%.*}} to [init] [[PROJECT]]
290+
//
291+
// CHECK: [[PROJECT:%.*]] = project_box [[BORROW_BOX]]
292+
// CHECK: [[CLS:%.*]] = mark_must_check [assignable_but_not_consumable] [[PROJECT]]
293+
// CHECK: [[BORROW:%.*]] = load_borrow [[CLS]]
279294
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly9borrowValyyAA2FDVhF :
280295
// CHECK: apply [[FN]]([[BORROW]])
281296
// CHECK: end_borrow [[BORROW]]
@@ -565,8 +580,11 @@ var booleanGuard2: Bool { false }
565580
// CHECK: br [[BB_CONT:bb[0-9]+]]
566581
//
567582
// CHECK: [[BB_E_2]]([[BBARG:%.*]] : @guaranteed
583+
// CHECK: [[NEW_BOX:%.*]] = alloc_box
584+
// CHECK: [[NEW_BOX_BORROW:%.*]] = begin_borrow [lexical] [[NEW_BOX]]
585+
// CHECK: [[NEW_BOX_PROJECT:%.*]] = project_box [[NEW_BOX_BORROW]]
568586
// CHECK: [[BBARG_COPY:%.*]] = copy_value [[BBARG]]
569-
// CHECK: [[NEW_VAL:%.*]] = move_value [lexical] [[BBARG_COPY]]
587+
// CHECK: store [[BBARG_COPY]] to [init] [[NEW_BOX_PROJECT]]
570588
// CHECK: end_borrow [[BORROWED_VALUE]]
571589
// CHECK: br [[BB_CONT]]
572590
//
@@ -597,8 +615,11 @@ var booleanGuard2: Bool { false }
597615
//
598616
// Move only case.
599617
// CHECK: [[BB_E2_RHS]]([[BBARG:%.*]] : @guaranteed
618+
// CHECK: [[NEW_BOX:%.*]] = alloc_box
619+
// CHECK: [[NEW_BOX_BORROW:%.*]] = begin_borrow [lexical] [[NEW_BOX]]
620+
// CHECK: [[NEW_BOX_PROJECT:%.*]] = project_box [[NEW_BOX_BORROW]]
600621
// CHECK: [[BBARG_COPY:%.*]] = copy_value [[BBARG]]
601-
// CHECK: move_value [lexical] [[BBARG_COPY]]
622+
// CHECK: store [[BBARG_COPY]] to [init] [[NEW_BOX_PROJECT]]
602623
// CHECK: end_borrow [[BORROWED_VALUE]]
603624
// CHECK: br [[BB_CONT]]
604625
//

0 commit comments

Comments
 (0)