Skip to content

Commit 20c2429

Browse files
gottesmmgottesmm
committed
[region-isolation] Do not look through begin_borrow or move_value if they are marked as a var_decl.
The reason why we do this is that we want to treat this as a separate value from their operand since they are the result of defining a new value. This has a few nice side-effects, one of which is that if a let results in just a begin_borrow [var_decl], we emit names for it. I also did a little work around helping variable name utils to lookup names from applies that are fed into a {move_value,begin_borrow} [var_decl] which then has the debug_value we are searching for.
1 parent b80faee commit 20c2429

File tree

5 files changed

+268
-23
lines changed

5 files changed

+268
-23
lines changed

‎lib/SILOptimizer/Analysis/RegionAnalysis.cpp‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,6 @@ static bool isStaticallyLookThroughInst(SILInstruction *inst) {
226226
default:
227227
return false;
228228
case SILInstructionKind::BeginAccessInst:
229-
case SILInstructionKind::BeginBorrowInst:
230229
case SILInstructionKind::BeginCOWMutationInst:
231230
case SILInstructionKind::BeginDeallocRefInst:
232231
case SILInstructionKind::BridgeObjectToRefInst:
@@ -249,7 +248,6 @@ static bool isStaticallyLookThroughInst(SILInstruction *inst) {
249248
case SILInstructionKind::MoveOnlyWrapperToCopyableAddrInst:
250249
case SILInstructionKind::MoveOnlyWrapperToCopyableBoxInst:
251250
case SILInstructionKind::MoveOnlyWrapperToCopyableValueInst:
252-
case SILInstructionKind::MoveValueInst:
253251
case SILInstructionKind::OpenExistentialAddrInst:
254252
case SILInstructionKind::OpenExistentialValueInst:
255253
case SILInstructionKind::ProjectBlockStorageInst:
@@ -269,6 +267,12 @@ static bool isStaticallyLookThroughInst(SILInstruction *inst) {
269267
case SILInstructionKind::UnmanagedToRefInst:
270268
case SILInstructionKind::InitExistentialValueInst:
271269
return true;
270+
case SILInstructionKind::MoveValueInst:
271+
// Look through if it isn't from a var decl.
272+
return !cast<MoveValueInst>(inst)->isFromVarDecl();
273+
case SILInstructionKind::BeginBorrowInst:
274+
// Look through if it isn't from a var decl.
275+
return !cast<BeginBorrowInst>(inst)->isFromVarDecl();
272276
case SILInstructionKind::UnconditionalCheckedCastInst: {
273277
auto cast = SILDynamicCastInst::getAs(inst);
274278
assert(cast);
@@ -2460,7 +2464,6 @@ CONSTANT_TRANSLATION(TupleInst, Assign)
24602464
// and whose operand and result are guaranteed to be mapped to the same
24612465
// underlying region.
24622466
CONSTANT_TRANSLATION(BeginAccessInst, LookThrough)
2463-
CONSTANT_TRANSLATION(BeginBorrowInst, LookThrough)
24642467
CONSTANT_TRANSLATION(BorrowedFromInst, LookThrough)
24652468
CONSTANT_TRANSLATION(BeginDeallocRefInst, LookThrough)
24662469
CONSTANT_TRANSLATION(BridgeObjectToRefInst, LookThrough)
@@ -2473,7 +2476,6 @@ CONSTANT_TRANSLATION(InitEnumDataAddrInst, LookThrough)
24732476
CONSTANT_TRANSLATION(OpenExistentialAddrInst, LookThrough)
24742477
CONSTANT_TRANSLATION(UncheckedRefCastInst, LookThrough)
24752478
CONSTANT_TRANSLATION(UpcastInst, LookThrough)
2476-
CONSTANT_TRANSLATION(MoveValueInst, LookThrough)
24772479
CONSTANT_TRANSLATION(MarkUnresolvedNonCopyableValueInst, LookThrough)
24782480
CONSTANT_TRANSLATION(MarkUnresolvedReferenceBindingInst, LookThrough)
24792481
CONSTANT_TRANSLATION(CopyableToMoveOnlyWrapperValueInst, LookThrough)
@@ -2742,6 +2744,20 @@ LOOKTHROUGH_IF_NONSENDABLE_RESULT_AND_OPERAND(UncheckedTakeEnumDataAddrInst)
27422744
// Custom Handling
27432745
//
27442746

2747+
TranslationSemantics
2748+
PartitionOpTranslator::visitMoveValueInst(MoveValueInst *mvi) {
2749+
if (mvi->isFromVarDecl())
2750+
return TranslationSemantics::Assign;
2751+
return TranslationSemantics::LookThrough;
2752+
}
2753+
2754+
TranslationSemantics
2755+
PartitionOpTranslator::visitBeginBorrowInst(BeginBorrowInst *bbi) {
2756+
if (bbi->isFromVarDecl())
2757+
return TranslationSemantics::Assign;
2758+
return TranslationSemantics::LookThrough;
2759+
}
2760+
27452761
TranslationSemantics PartitionOpTranslator::visitReturnInst(ReturnInst *ri) {
27462762
if (ri->getFunction()->getLoweredFunctionType()->hasTransferringResult()) {
27472763
return TranslationSemantics::TransferringNoResult;

‎lib/SILOptimizer/Utils/VariableNameUtils.cpp‎

Lines changed: 71 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,27 @@ SILValue VariableNameInferrer::findDebugInfoProvidingValuePhiArg(
320320
return SILValue();
321321
}
322322

323+
static BeginBorrowInst *hasOnlyBorrowingNonDestroyUse(SILValue searchValue) {
324+
BeginBorrowInst *result = nullptr;
325+
for (auto *use : searchValue->getUses()) {
326+
if (isIncidentalUse(use->getUser()))
327+
continue;
328+
if (use->isConsuming()) {
329+
if (!isa<DestroyValueInst>(use->getUser()))
330+
return nullptr;
331+
continue;
332+
}
333+
334+
auto *bbi = dyn_cast<BeginBorrowInst>(use->getUser());
335+
if (!bbi || !bbi->isFromVarDecl())
336+
return nullptr;
337+
if (result)
338+
return nullptr;
339+
result = bbi;
340+
}
341+
return result;
342+
}
343+
323344
SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper(
324345
SILValue searchValue, ValueSet &visitedValues) {
325346
assert(searchValue);
@@ -335,6 +356,56 @@ SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper(
335356

336357
LLVM_DEBUG(llvm::dbgs() << "Value: " << *searchValue);
337358

359+
// Before we do anything, lets see if we have an explicit match due to a
360+
// debug_value use.
361+
if (auto *use = getAnyDebugUse(searchValue)) {
362+
if (auto debugVar = DebugVarCarryingInst(use->getUser())) {
363+
assert(debugVar.getKind() == DebugVarCarryingInst::Kind::DebugValue);
364+
variableNamePath.push_back(use->getUser());
365+
366+
// We return the value, not the debug_info.
367+
return searchValue;
368+
}
369+
}
370+
371+
// If we are in Ownership SSA, see if we have an owned value that has one
372+
// use, a move_value [var decl]. In such a case, check the move_value [var
373+
// decl] for a debug_value.
374+
//
375+
// This pattern comes up if we are asked to get a name for an apply that is
376+
// used to initialize a value. The name will not yet be associated with the
377+
// value so we have to compensate.
378+
//
379+
// NOTE: This is a heuristic. Feel free to tweak accordingly.
380+
if (auto *singleUse = searchValue->getSingleUse()) {
381+
if (auto *mvi = dyn_cast<MoveValueInst>(singleUse->getUser())) {
382+
if (mvi->isFromVarDecl()) {
383+
if (auto *debugUse = getAnyDebugUse(mvi)) {
384+
if (auto debugVar = DebugVarCarryingInst(debugUse->getUser())) {
385+
assert(debugVar.getKind() ==
386+
DebugVarCarryingInst::Kind::DebugValue);
387+
variableNamePath.push_back(debugUse->getUser());
388+
389+
// We return the value, not the debug_info.
390+
return searchValue;
391+
}
392+
}
393+
}
394+
}
395+
}
396+
397+
if (auto *bbi = hasOnlyBorrowingNonDestroyUse(searchValue)) {
398+
if (auto *debugUse = getAnyDebugUse(bbi)) {
399+
if (auto debugVar = DebugVarCarryingInst(debugUse->getUser())) {
400+
assert(debugVar.getKind() == DebugVarCarryingInst::Kind::DebugValue);
401+
variableNamePath.push_back(debugUse->getUser());
402+
403+
// We return the value, not the debug_info.
404+
return searchValue;
405+
}
406+
}
407+
}
408+
338409
if (auto *allocInst = dyn_cast<AllocationInst>(searchValue)) {
339410
// If the instruction itself doesn't carry any variable info, see
340411
// whether it's copied from another place that does.
@@ -543,18 +614,6 @@ SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper(
543614
}
544615
}
545616

546-
// If we do not do an exact match, see if we can find a debug_var inst. If
547-
// we do, we always break since we have a root value.
548-
if (auto *use = getAnyDebugUse(searchValue)) {
549-
if (auto debugVar = DebugVarCarryingInst(use->getUser())) {
550-
assert(debugVar.getKind() == DebugVarCarryingInst::Kind::DebugValue);
551-
variableNamePath.push_back(use->getUser());
552-
553-
// We return the value, not the debug_info.
554-
return searchValue;
555-
}
556-
}
557-
558617
// Otherwise, try to see if we have a single value instruction we can look
559618
// through.
560619
if (isa<BeginBorrowInst>(searchValue) || isa<LoadInst>(searchValue) ||

‎test/Concurrency/transfernonsendable.swift‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,21 +1325,18 @@ func varSendableNonTrivialLetTupleFieldTest() async {
13251325
await transferToMain(test) // expected-tns-warning {{transferring 'test' may cause a data race}}
13261326
// expected-tns-note @-1 {{transferring disconnected 'test' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
13271327
// expected-complete-warning @-2 {{passing argument of non-sendable type '(Int, SendableKlass, NonSendableKlass)' into main actor-isolated context may introduce data races}}
1328-
let z = test.1
1328+
let z = test.1 // expected-tns-note {{use here could race}}
13291329
useValue(z)
1330-
useValue(test) // expected-tns-note {{use here could race}}
1330+
useValue(test)
13311331
}
13321332

13331333
func varNonSendableNonTrivialLetTupleFieldTest() async {
13341334
let test = (0, SendableKlass(), NonSendableKlass())
13351335
await transferToMain(test) // expected-tns-warning {{transferring 'test' may cause a data race}}
13361336
// expected-tns-note @-1 {{transferring disconnected 'test' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
13371337
// expected-complete-warning @-2 {{passing argument of non-sendable type '(Int, SendableKlass, NonSendableKlass)' into main actor-isolated context may introduce data races}}
1338-
let z = test.2
1339-
// The SIL emitted for the assignment of the tuple is just a jumble of
1340-
// instructions that are not semantically significant. The result is that we
1341-
// need a useValue here to actual provide something for the pass to chew on.
1342-
useValue(z) // expected-tns-note {{use here could race}}
1338+
let z = test.2 // expected-tns-note {{use here could race}}
1339+
useValue(z)
13431340
useValue(test)
13441341
}
13451342

‎test/Concurrency/transfernonsendable_instruction_matching.sil‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1694,3 +1694,65 @@ bb0(%0 : @guaranteed $ChildClass):
16941694
destroy_value %8 : $ChildClass
16951695
return %11 : $ChildClass
16961696
}
1697+
1698+
sil [ossa] @test_move_value_no_var_decl_is_lookthrough : $@convention(thin) @async () -> () {
1699+
bb0:
1700+
%constructFn = function_ref @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
1701+
%value = apply %constructFn() : $@convention(thin) () -> @owned NonSendableKlass
1702+
1703+
%transferNonSendableKlass = function_ref @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1704+
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferNonSendableKlass(%value) : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1705+
1706+
%value2 = move_value %value : $NonSendableKlass
1707+
destroy_value %value2 : $NonSendableKlass
1708+
1709+
%9999 = tuple ()
1710+
return %9999 : $()
1711+
}
1712+
1713+
sil [ossa] @test_move_value_var_decl_is_assign : $@convention(thin) @async () -> () {
1714+
bb0:
1715+
%constructFn = function_ref @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
1716+
%value = apply %constructFn() : $@convention(thin) () -> @owned NonSendableKlass
1717+
1718+
%transferNonSendableKlass = function_ref @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1719+
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferNonSendableKlass(%value) : $@convention(thin) @async (@guaranteed NonSendableKlass) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context}}
1720+
1721+
%value2 = move_value [var_decl] %value : $NonSendableKlass // expected-note {{use here could race}}
1722+
destroy_value %value2 : $NonSendableKlass
1723+
1724+
%9999 = tuple ()
1725+
return %9999 : $()
1726+
}
1727+
1728+
sil [ossa] @test_begin_borrow_no_var_decl_is_lookthrough : $@convention(thin) @async () -> () {
1729+
bb0:
1730+
%constructFn = function_ref @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
1731+
%value = apply %constructFn() : $@convention(thin) () -> @owned NonSendableKlass
1732+
1733+
%transferNonSendableKlass = function_ref @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1734+
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferNonSendableKlass(%value) : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1735+
1736+
%value2 = begin_borrow %value : $NonSendableKlass
1737+
end_borrow %value2 : $NonSendableKlass
1738+
destroy_value %value : $NonSendableKlass
1739+
1740+
%9999 = tuple ()
1741+
return %9999 : $()
1742+
}
1743+
1744+
sil [ossa] @test_begin_borrow_var_decl_is_assign : $@convention(thin) @async () -> () {
1745+
bb0:
1746+
%constructFn = function_ref @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
1747+
%value = apply %constructFn() : $@convention(thin) () -> @owned NonSendableKlass
1748+
1749+
%transferNonSendableKlass = function_ref @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
1750+
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferNonSendableKlass(%value) : $@convention(thin) @async (@guaranteed NonSendableKlass) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context}}
1751+
1752+
%value2 = begin_borrow [var_decl] %value : $NonSendableKlass // expected-note {{use here could race}}
1753+
end_borrow %value2 : $NonSendableKlass
1754+
destroy_value %value : $NonSendableKlass
1755+
1756+
%9999 = tuple ()
1757+
return %9999 : $()
1758+
}

‎test/SILOptimizer/variable_name_inference.sil‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -772,3 +772,114 @@ bb0(%0 : $*Klass):
772772
%9999 = tuple ()
773773
return %9999 : $()
774774
}
775+
776+
// CHECK-LABEL: begin running test 1 of 1 on move_value_var_decl: variable-name-inference with: @trace[0]
777+
// CHECK: Input Value: %2 = move_value [var_decl] %1 : $Klass
778+
// CHECK: Name: 'MyName'
779+
// CHECK: Root: %2 = move_value [var_decl] %1 : $Klass
780+
// CHECK: end running test 1 of 1 on move_value_var_decl: variable-name-inference with: @trace[0]
781+
sil [ossa] @move_value_var_decl : $@convention(thin) () -> () {
782+
bb0:
783+
specify_test "variable-name-inference @trace[0]"
784+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
785+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
786+
%2 = move_value [var_decl] %1 : $Klass
787+
debug_value %2 : $Klass, let, name "MyName"
788+
debug_value [trace] %2 : $Klass
789+
destroy_value %2 : $Klass
790+
%9999 = tuple ()
791+
return %9999 : $()
792+
}
793+
794+
// CHECK-LABEL: begin running test 1 of 1 on move_value_var_decl_2: variable-name-inference with: @trace[0]
795+
// CHECK: Input Value: %1 = apply %0() : $@convention(thin) () -> @owned Klass
796+
// CHECK: Name: 'MyName'
797+
// CHECK: Root: %1 = apply %0() : $@convention(thin) () -> @owned Klass
798+
// CHECK: end running test 1 of 1 on move_value_var_decl_2: variable-name-inference with: @trace[0]
799+
sil [ossa] @move_value_var_decl_2 : $@convention(thin) () -> () {
800+
bb0:
801+
specify_test "variable-name-inference @trace[0]"
802+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
803+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
804+
debug_value [trace] %1 : $Klass
805+
%2 = move_value [var_decl] %1 : $Klass
806+
debug_value %2 : $Klass, let, name "MyName"
807+
destroy_value %2 : $Klass
808+
%9999 = tuple ()
809+
return %9999 : $()
810+
}
811+
812+
// CHECK-LABEL: begin running test 1 of 1 on move_value_var_decl_3: variable-name-inference with: @trace[0]
813+
// CHECK-LABEL: Input Value: %1 = apply %0() : $@convention(thin) () -> @owned Klass
814+
// CHECK-LABEL: Name: 'unknown'
815+
// CHECK-LABEL: Root: 'unknown'
816+
// CHECK-LABEL: end running test 1 of 1 on move_value_var_decl_3: variable-name-inference with: @trace[0]
817+
sil [ossa] @move_value_var_decl_3 : $@convention(thin) () -> () {
818+
bb0:
819+
specify_test "variable-name-inference @trace[0]"
820+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
821+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
822+
debug_value [trace] %1 : $Klass
823+
%2 = move_value %1 : $Klass
824+
debug_value %2 : $Klass, let, name "MyName"
825+
destroy_value %2 : $Klass
826+
%9999 = tuple ()
827+
return %9999 : $()
828+
}
829+
830+
// CHECK-LABEL: begin running test 1 of 1 on begin_borrow_var_decl: variable-name-inference with: @trace[0]
831+
// CHECK: Input Value: %2 = begin_borrow [var_decl] %1 : $Klass
832+
// CHECK: Name: 'MyName'
833+
// CHECK: Root: %2 = begin_borrow [var_decl] %1 : $Klass
834+
// CHECK: end running test 1 of 1 on begin_borrow_var_decl: variable-name-inference with: @trace[0]
835+
sil [ossa] @begin_borrow_var_decl : $@convention(thin) () -> () {
836+
bb0:
837+
specify_test "variable-name-inference @trace[0]"
838+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
839+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
840+
%2 = begin_borrow [var_decl] %1 : $Klass
841+
debug_value %2 : $Klass, let, name "MyName"
842+
debug_value [trace] %2 : $Klass
843+
end_borrow %2 : $Klass
844+
destroy_value %1 : $Klass
845+
%9999 = tuple ()
846+
return %9999 : $()
847+
}
848+
849+
// CHECK-LABEL: begin running test 1 of 1 on begin_borrow_var_decl_2: variable-name-inference with: @trace[0]
850+
// CHECK: Input Value: %1 = apply %0() : $@convention(thin) () -> @owned Klass
851+
// CHECK: Name: 'MyName'
852+
// CHECK: Root: %1 = apply %0() : $@convention(thin) () -> @owned Klass
853+
// CHECK: end running test 1 of 1 on begin_borrow_var_decl_2: variable-name-inference with: @trace[0]
854+
sil [ossa] @begin_borrow_var_decl_2 : $@convention(thin) () -> () {
855+
bb0:
856+
specify_test "variable-name-inference @trace[0]"
857+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
858+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
859+
debug_value [trace] %1 : $Klass
860+
%2 = begin_borrow [var_decl] %1 : $Klass
861+
debug_value %2 : $Klass, let, name "MyName"
862+
end_borrow %2 : $Klass
863+
destroy_value %1 : $Klass
864+
%9999 = tuple ()
865+
return %9999 : $()
866+
}
867+
868+
// CHECK-LABEL: begin running test 1 of 1 on begin_borrow_var_decl_3: variable-name-inference with: @trace[0]
869+
// CHECK-LABEL: Input Value: %1 = apply %0() : $@convention(thin) () -> @owned Klass
870+
// CHECK-LABEL: Name: 'unknown'
871+
// CHECK-LABEL: Root: 'unknown'
872+
// CHECK-LABEL: end running test 1 of 1 on begin_borrow_var_decl_3: variable-name-inference with: @trace[0]
873+
sil [ossa] @begin_borrow_var_decl_3 : $@convention(thin) () -> () {
874+
bb0:
875+
specify_test "variable-name-inference @trace[0]"
876+
%0 = function_ref @getKlass : $@convention(thin) () -> @owned Klass
877+
%1 = apply %0() : $@convention(thin) () -> @owned Klass
878+
debug_value [trace] %1 : $Klass
879+
%2 = begin_borrow %1 : $Klass
880+
debug_value %2 : $Klass, let, name "MyName"
881+
end_borrow %2 : $Klass
882+
destroy_value %1 : $Klass
883+
%9999 = tuple ()
884+
return %9999 : $()
885+
}

0 commit comments

Comments
 (0)