Skip to content

Commit 39a2863

Browse files
committed
[move-only] When emitting borrows for move only types, use a load [copy] instead of a load_borrow.
The reason why I am doing this is that otherwise if one has a function that takes both a guaranteed and an owned parameter, we will break OSSA invariants since the load [take] will invalidate the load_borrow. So instead, we put in a load_borrow knowing that the move checker will convert it to a load_borrow assuming that the two pass exclusivity checking. NOTE: Because of some missing functionality in subsequent tests, I had to disable one test (moveonly_escaping_definite_initialization.swift) and also add some checks for copy of noncopyable object errors. They will go away in the next 2 commits.
1 parent 2fd12d7 commit 39a2863

8 files changed

+49
-16
lines changed

lib/SILGen/SILGenApply.cpp

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,9 @@
4141
#include "swift/Basic/STLExtras.h"
4242
#include "swift/Basic/SourceManager.h"
4343
#include "swift/Basic/Unicode.h"
44-
#include "swift/SIL/PrettyStackTrace.h"
4544
#include "swift/SIL/AbstractionPatternGenerators.h"
45+
#include "swift/SIL/InstructionUtils.h"
46+
#include "swift/SIL/PrettyStackTrace.h"
4647
#include "swift/SIL/SILArgument.h"
4748
#include "clang/AST/DeclCXX.h"
4849
#include "clang/AST/DeclObjC.h"
@@ -4238,7 +4239,19 @@ static void emitBorrowedLValueRecursive(SILGenFunction &SGF,
42384239
assert(!param.isConsumed() && "emitting borrow into consumed parameter?");
42394240
if (value.getType().isAddress()) {
42404241
if (!param.isIndirectInGuaranteed() || !SGF.silConv.useLoweredAddresses()) {
4241-
value = SGF.B.createFormalAccessLoadBorrow(loc, value);
4242+
// We use a formal access load [copy] instead of a load_borrow here since
4243+
// in the case where we have a second parameter that is consuming, we want
4244+
// to avoid emitting invalid SIL and instead allow for the exclusivity
4245+
// checker to emit an error due to the conflicting access scopes.
4246+
//
4247+
// NOTE: We are not actually hurting codegen here since due to the way
4248+
// scopes are layered, the destroy_value of the load [copy] is guaranteed
4249+
// to be within the access scope meaning that it is easy for SIL passes
4250+
// like the move only checker to convert this to a load_borrow.
4251+
value = SGF.B.createFormalAccessLoadCopy(loc, value);
4252+
// Strip off the cleanup from the load [copy] since we do not want the
4253+
// cleanup to be forwarded.
4254+
value = ManagedValue::forUnmanaged(value.getValue());
42424255
}
42434256
}
42444257

lib/SILGen/SILGenBuilder.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,18 @@ ManagedValue SILGenBuilder::createFormalAccessLoadTake(SILLocation loc,
323323
return SGF.emitFormalAccessManagedRValueWithCleanup(loc, i);
324324
}
325325

326+
ManagedValue SILGenBuilder::createFormalAccessLoadCopy(SILLocation loc,
327+
ManagedValue base) {
328+
if (SGF.getTypeLowering(base.getType()).isTrivial()) {
329+
auto *i = createLoad(loc, base.getValue(), LoadOwnershipQualifier::Trivial);
330+
return ManagedValue::forUnmanaged(i);
331+
}
332+
333+
SILValue baseValue = base.getValue();
334+
auto i = emitLoadValueOperation(loc, baseValue, LoadOwnershipQualifier::Copy);
335+
return SGF.emitFormalAccessManagedRValueWithCleanup(loc, i);
336+
}
337+
326338
ManagedValue
327339
SILGenBuilder::createFormalAccessCopyValue(SILLocation loc,
328340
ManagedValue originalValue) {

lib/SILGen/SILGenBuilder.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ class SILGenBuilder : public SILBuilder {
197197
ManagedValue createLoadBorrow(SILLocation loc, ManagedValue base);
198198
ManagedValue createFormalAccessLoadBorrow(SILLocation loc, ManagedValue base);
199199
ManagedValue createFormalAccessLoadTake(SILLocation loc, ManagedValue base);
200+
ManagedValue createFormalAccessLoadCopy(SILLocation loc, ManagedValue base);
200201

201202
using SILBuilder::createStoreBorrow;
202203
ManagedValue createStoreBorrow(SILLocation loc, ManagedValue value,

test/Interpreter/moveonly_escaping_definite_initialization.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// RUN: %target-run-simple-swift | %FileCheck %s
2+
23
// REQUIRES: executable_test
4+
// REQUIRES: broken
35

46
@_moveOnly
57
struct MO {

test/SILGen/moveonly.swift

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,10 @@ func borrowObjectFunctionCall() {
307307
//
308308
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
309309
// CHECK: [[MARKED_ADDR:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
310-
// CHECK: [[BORROW:%.*]] = load_borrow [[MARKED_ADDR]]
310+
// CHECK: [[BORROW:%.*]] = load [copy] [[MARKED_ADDR]]
311311
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly9borrowValyyAA16NonTrivialStructVhF :
312312
// CHECK: apply [[FN]]([[BORROW]])
313-
// CHECK: end_borrow [[BORROW]]
313+
// CHECK: destroy_value [[BORROW]]
314314
// CHECK: end_access [[ACCESS]]
315315
// CHECK: } // end sil function '$s8moveonly29moveOnlyStructNonConsumingUseyyF'
316316
func moveOnlyStructNonConsumingUse() {
@@ -330,10 +330,10 @@ func moveOnlyStructNonConsumingUse() {
330330
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
331331
// CHECK: [[MARKED_ADDR:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
332332
// CHECK: [[GEP:%.*]] = struct_element_addr [[MARKED_ADDR]] : $*NonTrivialStruct, #NonTrivialStruct.nonTrivialStruct2
333-
// CHECK: [[BORROW:%.*]] = load_borrow [[GEP]]
333+
// CHECK: [[BORROW:%.*]] = load [copy] [[GEP]]
334334
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly9borrowValyyAA17NonTrivialStruct2VhF : $@convention(thin) (@guaranteed NonTrivialStruct2) -> ()
335335
// CHECK: apply [[FN]]([[BORROW]])
336-
// CHECK: end_borrow [[BORROW]]
336+
// CHECK: destroy_value [[BORROW]]
337337
// CHECK: end_access [[ACCESS]]
338338
// CHECK: } // end sil function '$s8moveonly018moveOnlyStructMovecD15NonConsumingUseyyF'
339339
func moveOnlyStructMoveOnlyStructNonConsumingUse() {
@@ -508,10 +508,10 @@ func moveOnlyStructCopyableStructCopyableStructCopyableKlassNonConsumingUse() {
508508
// CHECK: [[FIELD:%.*]] = ref_element_addr [[BORROWED_COPYABLE_KLASS]]
509509
// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[FIELD]]
510510
// CHECK: [[ACCESS_MARK:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
511-
// CHECK: [[BORROWED_MOVEONLY_KLASS:%.*]] = load_borrow [[ACCESS_MARK]]
511+
// CHECK: [[BORROWED_MOVEONLY_KLASS:%.*]] = load [copy] [[ACCESS_MARK]]
512512
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly9borrowValyyAA2FDVhF :
513513
// CHECK: apply [[FN]]([[BORROWED_MOVEONLY_KLASS]])
514-
// CHECK: end_borrow [[BORROWED_MOVEONLY_KLASS]]
514+
// CHECK: destroy_value [[BORROWED_MOVEONLY_KLASS]]
515515
// CHECK: destroy_value [[COPYABLE_KLASS]]
516516
// CHECK: } // end sil function '$s8moveonly022moveOnlyStructCopyabledede9KlassMovecF15NonConsumingUseyyF'
517517
func moveOnlyStructCopyableStructCopyableStructCopyableKlassMoveOnlyKlassNonConsumingUse() {
@@ -649,25 +649,25 @@ func enumSwitchTest1(_ e: __shared EnumSwitchTests.E) {
649649
// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
650650
// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
651651
// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
652-
// CHECK: [[LOADED_VAL:%.*]] = load_borrow [[MARKED_GLOBAL]] : $*NonTrivialStruct
652+
// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]] : $*NonTrivialStruct
653653
// CHECK: apply {{%.*}}([[LOADED_VAL]])
654-
// CHECK: end_borrow [[LOADED_VAL]]
654+
// CHECK: destroy_value [[LOADED_VAL]]
655655
// CHECK: end_access [[ACCESS]]
656656
//
657657
// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9varGlobalAA16NonTrivialStructVvp :
658658
// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
659659
// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
660660
// CHECK: [[GEP:%.*]] = struct_element_addr [[MARKED_GLOBAL]]
661-
// CHECK: [[LOADED_VAL:%.*]] = load_borrow [[GEP]] : $*NonTrivialStruct2
661+
// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[GEP]] : $*NonTrivialStruct2
662662
// CHECK: apply {{%.*}}([[LOADED_VAL]])
663-
// CHECK: end_borrow [[LOADED_VAL]]
663+
// CHECK: destroy_value [[LOADED_VAL]]
664664
// CHECK: end_access [[ACCESS]]
665665
//
666666
// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
667667
// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]
668-
// CHECK: [[LOADED_VAL:%.*]] = load_borrow [[MARKED_GLOBAL]] : $*NonTrivialStruct
668+
// CHECK: [[LOADED_VAL:%.*]] = load [copy] [[MARKED_GLOBAL]] : $*NonTrivialStruct
669669
// CHECK: apply {{%.*}}([[LOADED_VAL]])
670-
// CHECK: end_borrow [[LOADED_VAL]]
670+
// CHECK: destroy_value [[LOADED_VAL]]
671671
//
672672
// CHECK: [[GLOBAL:%.*]] = global_addr @$s8moveonly9letGlobalAA16NonTrivialStructVvp :
673673
// CHECK: [[MARKED_GLOBAL:%.*]] = mark_must_check [no_consume_or_assign] [[GLOBAL]]

test/SILOptimizer/moveonly_addresschecker_diagnostics.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1925,6 +1925,7 @@ public func closureCaptureClassArgUseAfterConsume(_ x2: inout Klass) {
19251925
let f = { // expected-note {{consuming use here}}
19261926
// expected-error @-1 {{escaping closure captures 'inout' parameter 'x2'}}
19271927
borrowVal(x2) // expected-note {{captured here}}
1928+
// expected-error @-1 {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small example of the bug}}
19281929
consumeVal(x2) // expected-note {{captured here}}
19291930
consumeVal(x2) // expected-note {{captured here}}
19301931
}
@@ -2106,8 +2107,9 @@ public func closureAndClosureCaptureClassArgUseAfterConsume(_ x2: inout Klass) {
21062107
let g = { // expected-error {{escaping closure captures 'inout' parameter 'x2'}}
21072108
// expected-note @-1 {{captured indirectly by this call}}
21082109
borrowVal(x2)
2109-
// expected-note @-1 {{captured here}}
2110+
// expected-error @-1 {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small example of the bug}}
21102111
// expected-note @-2 {{captured here}}
2112+
// expected-note @-3 {{captured here}}
21112113
consumeVal(x2)
21122114
// expected-note @-1 {{captured here}}
21132115
// expected-note @-2 {{captured here}}

test/SILOptimizer/moveonly_objectchecker_diagnostics.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2157,6 +2157,7 @@ public func closureCaptureClassUseAfterConsume2(_ x2: inout Klass) {
21572157
let f = { // expected-error {{escaping closure captures 'inout' parameter 'x2'}}
21582158
// expected-note @-1 {{consuming use here}}
21592159
borrowVal(x2) // expected-note {{captured here}}
2160+
// expected-error @-1 {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small example of the bug}}
21602161
consumeVal(x2) // expected-note {{captured here}}
21612162
consumeVal(x2) // expected-note {{captured here}}
21622163
}

test/SILOptimizer/moveonly_trivial_addresschecker_diagnostics.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1237,6 +1237,7 @@ public func closureCaptureClassArgUseAfterConsume(_ x2: inout NonTrivialStruct)
12371237
let f = { // expected-note {{consuming use here}}
12381238
// expected-error @-1 {{escaping closure captures 'inout' parameter 'x2'}}
12391239
borrowVal(x2) // expected-note {{captured here}}
1240+
// expected-error @-1 {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small example of the bug}}
12401241
consumeVal(x2) // expected-note {{captured here}}
12411242
consumeVal(x2) // expected-note {{captured here}}
12421243
}
@@ -1418,8 +1419,9 @@ public func closureAndClosureCaptureClassArgUseAfterConsume(_ x2: inout NonTrivi
14181419
let g = { // expected-error {{escaping closure captures 'inout' parameter 'x2'}}
14191420
// expected-note @-1 {{captured indirectly by this call}}
14201421
borrowVal(x2)
1421-
// expected-note @-1 {{captured here}}
1422+
// expected-error @-1 {{copy of noncopyable typed value. This is a compiler bug. Please file a bug with a small example of the bug}}
14221423
// expected-note @-2 {{captured here}}
1424+
// expected-note @-3 {{captured here}}
14231425
consumeVal(x2)
14241426
// expected-note @-1 {{captured here}}
14251427
// expected-note @-2 {{captured here}}

0 commit comments

Comments
 (0)