Skip to content

Commit cafdf05

Browse files
atrickbob-wilson
atrick
authored and
bob-wilson
committed
Disable merging of read/modify exclusivity access markers.
When two access scopes have different access types, it is not safe to merge them into one access. Doing so will introduce false conflicts which manifest as crashes in user code. To do this optimization, we would need additional data flow information about *potential* read conflicts before converting a read to a modify access. Fixes rdar://48239213: Fatal access conflict detected.
1 parent 8b02a61 commit cafdf05

File tree

3 files changed

+252
-116
lines changed

3 files changed

+252
-116
lines changed

‎lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -988,6 +988,12 @@ static void mergeEndAccesses(BeginAccessInst *parentIns,
988988
}
989989

990990
static bool canMergeEnd(BeginAccessInst *parentIns, BeginAccessInst *childIns) {
991+
// A [read] access cannot be converted to a [modify] without potentially
992+
// introducing new conflicts that were previously ignored. Merging read/modify
993+
// will require additional data flow information.
994+
if (childIns->getAccessKind() != parentIns->getAccessKind())
995+
return false;
996+
991997
auto *endP = getSingleEndAccess(parentIns);
992998
if (!endP)
993999
return false;
@@ -1087,13 +1093,6 @@ static bool mergeAccesses(
10871093
LLVM_DEBUG(llvm::dbgs()
10881094
<< "Merging: " << *childIns << " into " << *parentIns << "\n");
10891095

1090-
// Change the type of access of parent:
1091-
// should be the worse between it and child
1092-
auto childAccess = childIns->getAccessKind();
1093-
if (parentIns->getAccessKind() < childAccess) {
1094-
parentIns->setAccessKind(childAccess);
1095-
}
1096-
10971096
// Change the no nested conflict of parent:
10981097
// should be the worst case scenario: we might merge to non-conflicting
10991098
// scopes to a conflicting one. f the new result does not conflict,

‎test/SILOptimizer/access_enforcement_opts.sil

Lines changed: 117 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -506,14 +506,19 @@ bb0(%0 : ${ var Int64 }):
506506
// }
507507
// Preserve the scope of the outer inout access. Runtime trap expected.
508508
//
509+
// FIXME: The optimization should be able to merge these accesses, but
510+
// it must first prove that no other conflicting read accesses occur
511+
// within the existing read access scopes.
512+
//
509513
// CHECK-LABEL: sil @$s17enforce_with_opts24testInoutWriteEscapeReadyyF : $@convention(thin) () -> () {
510514
// CHECK: [[BOX:%.*]] = alloc_box ${ var Int64 }, var, name "x"
511515
// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var Int64 }, 0
512516
// CHECK: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[BOXADR]] : $*Int64
513517
// CHECK: apply
514-
// CHECK-NOT: begin_access
515518
// CHECK: end_access [[BEGIN]]
516-
// CHECK-NOT: begin_access
519+
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[BOXADR]] : $*Int64
520+
// CHECK: load [[BEGIN2]]
521+
// CHECK: end_access [[BEGIN2]]
517522
// CHECK-LABEL: } // end sil function '$s17enforce_with_opts24testInoutWriteEscapeReadyyF'
518523
sil @$s17enforce_with_opts24testInoutWriteEscapeReadyyF : $@convention(thin) () -> () {
519524
bb0:
@@ -579,14 +584,19 @@ bb0(%0 : ${ var Int64 }):
579584
// }
580585
// Preserve the scope of the outer inout access. Runtime trap expected.
581586
//
587+
// FIXME: The optimization should be able to merge these accesses, but
588+
// it must first prove that no other conflicting read accesses occur
589+
// within the existing read access scopes.
590+
//
582591
// CHECK-LABEL: sil @$s17enforce_with_opts020testInoutWriteEscapeF0yyF : $@convention(thin) () -> () {
583592
// CHECK: [[BOX:%.*]] = alloc_box ${ var Int64 }, var, name "x"
584593
// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var Int64 }, 0
585594
// CHECK: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[BOXADR]] : $*Int64
586595
// CHECK: apply
587-
// CHECK-NOT: begin_access
588596
// CHECK: end_access [[BEGIN]]
589-
// CHECK-NOT: begin_access
597+
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[BOXADR]] : $*Int64
598+
// CHECK: load [[BEGIN2]]
599+
// CHECK: end_access [[BEGIN2]]
590600
// CHECK-LABEL: } // end sil function '$s17enforce_with_opts020testInoutWriteEscapeF0yyF'
591601
sil @$s17enforce_with_opts020testInoutWriteEscapeF0yyF : $@convention(thin) () -> () {
592602
bb0:
@@ -999,14 +1009,21 @@ bb0:
9991009
// public func testOldToNewMapWrite) {
10001010
// Checks merging of 3 scopes resulting in a larger modify scope
10011011
//
1012+
// FIXME: The optimization should be able to merge these accesses, but
1013+
// it must first prove that no other conflicting read accesses occur
1014+
// within the existing read access scopes.
1015+
//
10021016
// CHECK-LABEL: sil @testOldToNewMapWrite : $@convention(thin) () -> () {
10031017
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1004-
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
1005-
// CHECK-NEXT: load [[BEGIN]] : $*X
1006-
// CHECK: store {{.*}} to [[BEGIN]] : $*X
1018+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
10071019
// CHECK-NEXT: load [[BEGIN]] : $*X
10081020
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1009-
// CHECK-NOT: begin_access
1021+
// CHECK: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1022+
// CHECK-NEXT: store {{.*}} to [[BEGIN2]] : $*X
1023+
// CHECK-NEXT: end_access [[BEGIN2]] : $*X
1024+
// CHECK: [[BEGIN3:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1025+
// CHECK-NEXT: load [[BEGIN3]] : $*X
1026+
// CHECK-NEXT: end_access [[BEGIN3]] : $*X
10101027
// CHECK-LABEL: } // end sil function 'testOldToNewMapWrite'
10111028
sil @testOldToNewMapWrite : $@convention(thin) () -> () {
10121029
bb0:
@@ -1031,15 +1048,20 @@ bb0:
10311048
// public func testDataFlowAcrossBBs() {
10321049
// Checks merging of scopes across basic blocks - propagating that information
10331050
//
1051+
// FIXME: The optimization should be able to merge these accesses, but
1052+
// it must first prove that no other conflicting read accesses occur
1053+
// within the existing read access scopes.
1054+
//
10341055
// CHECK-LABEL: sil @testDataFlowAcrossBBs : $@convention(thin) () -> () {
10351056
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1036-
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
1057+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
10371058
// CHECK-NEXT: load [[BEGIN]] : $*X
1059+
// CHECK-NEXT: end_access [[BEGIN]] : $*X
10381060
// CHECK-NEXT: br bb1
10391061
// CHECK: br bb2
1040-
// CHECK: load [[BEGIN]] : $*X
1041-
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1042-
// CHECK-NOT: begin_access
1062+
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1063+
// CHECK-NEXT: load [[BEGIN2]] : $*X
1064+
// CHECK-NEXT: end_access [[BEGIN2]] : $*X
10431065
// CHECK-LABEL: } // end sil function 'testDataFlowAcrossBBs'
10441066
sil @testDataFlowAcrossBBs : $@convention(thin) () -> () {
10451067
bb0:
@@ -1063,15 +1085,20 @@ bb2:
10631085
// public func testDataFlowAcrossInnerLoop() {
10641086
// Checks merging of scopes across an inner loop
10651087
//
1088+
// FIXME: The optimization should be able to merge these accesses, but
1089+
// it must first prove that no other conflicting read accesses occur
1090+
// within the existing read access scopes.
1091+
//
10661092
// CHECK-LABEL: sil @testDataFlowAcrossInnerLoop : $@convention(thin) () -> () {
10671093
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1068-
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
1094+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
10691095
// CHECK-NEXT: load [[BEGIN]] : $*X
1096+
// CHECK-NEXT: end_access [[BEGIN]] : $*X
10701097
// CHECK-NEXT: br bb1
10711098
// CHECK: cond_br {{.*}}, bb1, bb2
1072-
// CHECK: load [[BEGIN]] : $*X
1073-
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1074-
// CHECK-NOT: begin_access
1099+
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1100+
// CHECK-NEXT: load [[BEGIN2]] : $*X
1101+
// CHECK-NEXT: end_access [[BEGIN2]] : $*X
10751102
// CHECK-LABEL: } // end sil function 'testDataFlowAcrossInnerLoop'
10761103
sil @testDataFlowAcrossInnerLoop : $@convention(thin) () -> () {
10771104
bb0:
@@ -1248,13 +1275,19 @@ bb4:
12481275
// Checks detection of irreducible control flow / bail + parts that we *can* merge
12491276
// See disableCrossBlock in the algorithm: this detects this corner case
12501277
//
1278+
// FIXME: The optimization should be able to merge these accesses, but
1279+
// it must first prove that no other conflicting read accesses occur
1280+
// within the existing read access scopes.
1281+
//
12511282
// CHECK-LABEL: sil @testIrreducibleGraph2 : $@convention(thin) () -> () {
12521283
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1253-
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
1284+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
12541285
// CHECK-NEXT: load [[BEGIN]] : $*X
1255-
// CHECK-NEXT: br bb1
1256-
// CHECK: load [[BEGIN]] : $*X
12571286
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1287+
// CHECK-NEXT: br bb1
1288+
// CHECK: [[BEGIN1:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1289+
// CHECK-NEXT: load [[BEGIN1]] : $*X
1290+
// CHECK-NEXT: end_access [[BEGIN1]] : $*X
12581291
// CHECK: cond_br {{.*}}, bb2, bb3
12591292
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
12601293
// CHECK-NEXT: load [[BEGIN2]] : $*X
@@ -1390,13 +1423,19 @@ bb0(%0 : $RefElemNoConflictClass):
13901423
// During the merge optimization,
13911424
// Check that we don't merge cross strongly component boundaries for now
13921425
//
1426+
// FIXME: The optimization should be able to merge these accesses, but
1427+
// it must first prove that no other conflicting read accesses occur
1428+
// within the existing read access scopes.
1429+
//
13931430
// CHECK-LABEL: sil @testStronglyConnectedComponent : $@convention(thin) () -> () {
13941431
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1395-
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
1432+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
13961433
// CHECK-NEXT: load [[BEGIN]] : $*X
1397-
// CHECK-NEXT: br bb1
1398-
// CHECK: load [[BEGIN]] : $*X
13991434
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1435+
// CHECK-NEXT: br bb1
1436+
// CHECK: [[BEGIN1:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
1437+
// CHECK-NEXT: load [[BEGIN1]] : $*X
1438+
// CHECK-NEXT: end_access [[BEGIN1]] : $*X
14001439
// CHECK: cond_br {{.*}}, bb2, bb3
14011440
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
14021441
// CHECK-NEXT: load [[BEGIN2]] : $*X
@@ -1555,3 +1594,59 @@ bb3(%1 : $*X):
15551594
return %10 : $()
15561595
}
15571596

1597+
// --- rdar://48239213: Fatal access conflict detected.
1598+
//
1599+
// The read/modify pair of accesses in testReadModifyConflictPair
1600+
// cannot be merged without introducing a false conflict.
1601+
1602+
public class TestClass {
1603+
@_hasStorage @_hasInitialValue var flags: Int64 { get set }
1604+
}
1605+
1606+
// CHECK-LABEL: sil hidden [noinline] @readFlags : $@convention(method) (Int64, @guaranteed TestClass) -> Bool {
1607+
// CHECK: bb0(%0 : $Int64, %1 : $TestClass):
1608+
// CHECK: [[ADR:%.*]] = ref_element_addr %1 : $TestClass, #TestClass.flags
1609+
// CHECK: begin_access [read] [dynamic] [no_nested_conflict] [[ADR]] : $*Int64
1610+
// CHECK: load %4 : $*Builtin.Int64
1611+
// CHECK: end_access
1612+
// CHECK-LABEL: } // end sil function 'readFlags'
1613+
sil hidden [noinline] @readFlags : $@convention(method) (Int64, @guaranteed TestClass) -> Bool {
1614+
bb0(%0 : $Int64, %1 : $TestClass):
1615+
%2 = ref_element_addr %1 : $TestClass, #TestClass.flags
1616+
%3 = begin_access [read] [dynamic] [no_nested_conflict] %2 : $*Int64
1617+
%4 = struct_element_addr %3 : $*Int64, #Int64._value
1618+
%5 = load %4 : $*Builtin.Int64
1619+
end_access %3 : $*Int64
1620+
%7 = struct_extract %0 : $Int64, #Int64._value
1621+
%8 = builtin "cmp_eq_Int64"(%5 : $Builtin.Int64, %7 : $Builtin.Int64) : $Builtin.Int1
1622+
%9 = struct $Bool (%8 : $Builtin.Int1)
1623+
return %9 : $Bool
1624+
}
1625+
1626+
// CHECK-LABEL: sil @testReadModifyConflictPair : $@convention(method) (@guaranteed TestClass) -> () {
1627+
// CHECK: bb0(%0 : $TestClass):
1628+
// CHECK: [[ADR:%.*]] = ref_element_addr %0 : $TestClass, #TestClass.flags
1629+
// CHECK: begin_access [read] [dynamic] [no_nested_conflict] [[ADR]] : $*Int64
1630+
// CHECK: load
1631+
// CHECK: end_access
1632+
// CHECK: apply {{.*}} : $@convention(method) (Int64, @guaranteed TestClass) -> Bool
1633+
// CHECK: begin_access [modify] [dynamic] [no_nested_conflict] [[ADR]] : $*Int64
1634+
// CHECK: store
1635+
// CHECK: end_access
1636+
// CHECK-LABEL: } // end sil function 'testReadModifyConflictPair'
1637+
sil @testReadModifyConflictPair : $@convention(method) (@guaranteed TestClass) -> () {
1638+
bb0(%0 : $TestClass):
1639+
%1 = ref_element_addr %0 : $TestClass, #TestClass.flags
1640+
%2 = begin_access [read] [dynamic] %1 : $*Int64
1641+
%3 = load %2 : $*Int64
1642+
end_access %2 : $*Int64
1643+
%5 = function_ref @readFlags : $@convention(method) (Int64, @guaranteed TestClass) -> Bool
1644+
%6 = apply %5(%3, %0) : $@convention(method) (Int64, @guaranteed TestClass) -> Bool
1645+
%7 = integer_literal $Builtin.Int64, 3
1646+
%8 = struct $Int64 (%7 : $Builtin.Int64)
1647+
%9 = begin_access [modify] [dynamic] %1 : $*Int64
1648+
store %8 to %9 : $*Int64
1649+
end_access %9 : $*Int64
1650+
%12 = tuple ()
1651+
return %12 : $()
1652+
}

0 commit comments

Comments
 (0)