Skip to content

Commit 02b872e

Browse files
swift-ciswift-ci
authored
Merge pull request #19372 from shajrawi/arrayinclass
2 parents eebeae9 + 43efed3 commit 02b872e

File tree

2 files changed

+121
-22
lines changed

2 files changed

+121
-22
lines changed

‎lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,26 @@ void AccessConflictAndMergeAnalysis::visitBeginAccess(
654654
// It can potentially be folded
655655
// regardless of whether it may conflict with an outer access.
656656
addInScopeAccess(info, beginAccess);
657+
// We can merge out-of-scope regardless of having a conflict within a scope,
658+
// normally, it would have made more sense to add it to out-of-scope set
659+
// *only* after encountering the end_access instruction.
660+
// However, that will lose us some valid optimization potential:
661+
// consider the following pseudo-SIL:
662+
// begin_access %x
663+
// end_access %x
664+
// begin_access %x
665+
// conflict
666+
// end_access %x
667+
// we can merge both of these scopes
668+
// but, if we only add the instr. after seeing end_access,
669+
// then we would not have the first begin_access in out-of-scope
670+
// set when encoutnering the 2nd end_access due to "conflict"
671+
// NOTE: What we really want to do here is to check if
672+
// we should add the new beginAccess to 'mergePairs' structure
673+
// the reason for calling this method is to check for that.
674+
// logically, we only need to add an instructio to
675+
// out-of-scope conflict-free set when we visit end_access
676+
addOutOfScopeAccess(info, beginAccess);
657677
}
658678

659679
void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
@@ -669,10 +689,18 @@ void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
669689
removeInScopeAccess(info, beginAccess);
670690
}
671691

672-
// We can merge out-of-scope regardless of having a conflict within a scope,
673-
// when encountering an end access instruction,
674-
// regardless of having it in the In scope set,
675-
// add it to the out of scope set.
692+
// If this exact instruction is already in out-of-scope - skip:
693+
if (info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.count(
694+
beginAccess) > 0) {
695+
return;
696+
}
697+
// Else we have the opposite situation to the one described in
698+
// visitBeginAccess: the first scope is the one conflicting while the second
699+
// does not - begin_access %x conflict end_access %x begin_access %x
700+
// end_access %x
701+
// when seeing the conflict we remove the first begin instruction
702+
// but, we can still merge those scopes *UNLESS* there's a conflict
703+
// between the first end_access and the second begin_access
676704
LLVM_DEBUG(llvm::dbgs() << "Got out of scope from " << *beginAccess << " to "
677705
<< *endAccess << "\n");
678706

‎test/SILOptimizer/access_enforcement_opts.sil

Lines changed: 89 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -48,28 +48,17 @@ bb0(%0 : $@thin X.Type):
4848
// }
4949
// Preserve begin/end scope for nested conflicts,
5050
// after inlining (read|modify)AndPerform.
51+
// need to split it into 3 SIL functions else merging would kick in and we would be testing something else
5152
//
52-
// CHECK-LABEL: sil hidden @testNestedAccess : $@convention(thin) () -> () {
53+
// CHECK-LABEL: sil hidden @testNestedAccess1 : $@convention(thin) () -> () {
5354
// CHECK: [[F1:%.*]] = function_ref @testNestedAccessClosure1 : $@convention(thin) () -> ()
5455
// CHECK: [[C1:%.*]] = convert_function [[F1]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
5556
// CHECK: [[TF1:%.*]] = thin_to_thick_function [[C1]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed ()
5657
// CHECK: [[A1:%.*]] = begin_access [read] [dynamic] %0 : $*X
5758
// CHECK: apply [[TF1]]() : $@noescape @callee_guaranteed () -> ()
5859
// CHECK: end_access [[A1]] : $*X
59-
// CHECK: [[F2:%.*]] = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
60-
// CHECK: [[C2:%.*]] = convert_function [[F2]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
61-
// CHECK: [[TF2:%.*]] = thin_to_thick_function [[C2]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
62-
// CHECK: [[A2:%.*]] = begin_access [modify] [dynamic] %0 : $*X
63-
// CHECK: apply [[TF2]]() : $@noescape @callee_guaranteed () -> ()
64-
// CHECK: end_access [[A2]] : $*X
65-
// CHECK: [[F3:%.*]] = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
66-
// CHECK: [[C3:%.*]] = convert_function [[F3]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
67-
// CHECK: [[TF3:%.*]] = thin_to_thick_function [[C3]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
68-
// CHECK: [[A3:%.*]] = begin_access [modify] [dynamic] %0 : $*X
69-
// CHECK: apply [[TF3]]() : $@noescape @callee_guaranteed () -> ()
70-
// CHECK: end_access [[A3]] : $*X
71-
// CHECK-LABEL: } // end sil function 'testNestedAccess'
72-
sil hidden @testNestedAccess : $@convention(thin) () -> () {
60+
// CHECK-LABEL: } // end sil function 'testNestedAccess1'
61+
sil hidden @testNestedAccess1 : $@convention(thin) () -> () {
7362
bb0:
7463
%2 = global_addr @globalX: $*X
7564
%3 = function_ref @testNestedAccessClosure1 : $@convention(thin) () -> ()
@@ -78,14 +67,40 @@ bb0:
7867
%6 = begin_access [read] [dynamic] %2 : $*X
7968
%9 = apply %5() : $@noescape @callee_guaranteed () -> ()
8069
end_access %6 : $*X
81-
70+
%36 = tuple ()
71+
return %36 : $()
72+
}
73+
// CHECK-LABEL: sil hidden @testNestedAccess2 : $@convention(thin) () -> () {
74+
// CHECK: [[F2:%.*]] = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
75+
// CHECK: [[C2:%.*]] = convert_function [[F2]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
76+
// CHECK: [[TF2:%.*]] = thin_to_thick_function [[C2]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
77+
// CHECK: [[A2:%.*]] = begin_access [modify] [dynamic] %0 : $*X
78+
// CHECK: apply [[TF2]]() : $@noescape @callee_guaranteed () -> ()
79+
// CHECK: end_access [[A2]] : $*X
80+
// CHECK-LABEL: } // end sil function 'testNestedAccess2'
81+
sil hidden @testNestedAccess2 : $@convention(thin) () -> () {
82+
bb0:
83+
%2 = global_addr @globalX: $*X
8284
%15 = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
8385
%16 = convert_function %15 : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
8486
%17 = thin_to_thick_function %16 : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
8587
%18 = begin_access [modify] [dynamic] %2 : $*X
8688
%21 = apply %17() : $@noescape @callee_guaranteed () -> ()
8789
end_access %18 : $*X
88-
90+
%36 = tuple ()
91+
return %36 : $()
92+
}
93+
// CHECK-LABEL: sil hidden @testNestedAccess3 : $@convention(thin) () -> () {
94+
// CHECK: [[F3:%.*]] = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
95+
// CHECK: [[C3:%.*]] = convert_function [[F3]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
96+
// CHECK: [[TF3:%.*]] = thin_to_thick_function [[C3]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
97+
// CHECK: [[A3:%.*]] = begin_access [modify] [dynamic] %0 : $*X
98+
// CHECK: apply [[TF3]]() : $@noescape @callee_guaranteed () -> ()
99+
// CHECK: end_access [[A3]] : $*X
100+
// CHECK-LABEL: } // end sil function 'testNestedAccess3'
101+
sil hidden @testNestedAccess3 : $@convention(thin) () -> () {
102+
bb0:
103+
%2 = global_addr @globalX: $*X
89104
%27 = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
90105
%28 = convert_function %27 : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
91106
%29 = thin_to_thick_function %28 : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
@@ -1306,6 +1321,8 @@ class RefElemClass {
13061321
init()
13071322
}
13081323

1324+
// Checks that we don't crash when unable to create projection paths
1325+
// we can't merge anything here because of that
13091326
// CHECK-LABEL: sil @ref_elem_c : $@convention(thin) (RefElemClass) -> () {
13101327
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
13111328
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
@@ -1314,8 +1331,10 @@ class RefElemClass {
13141331
// CHECK-NEXT: [[REFX:%.*]] = ref_element_addr %0 : $RefElemClass, #RefElemClass.x
13151332
// CHECK-NEXT: [[REFY:%.*]] = ref_element_addr %0 : $RefElemClass, #RefElemClass.y
13161333
// CHECK-NEXT: [[BEGINX:%.*]] = begin_access [modify] [dynamic] [[REFX]] : $*BitfieldOne
1334+
// CHECK: end_access [[BEGINX]] : $*BitfieldOne
13171335
// CHECK: [[BEGINY:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[REFY]] : $*Int32
1318-
// CHECK-NEXT: end_access [[BEGINX]] : $*BitfieldOne
1336+
// CHECK: [[BEGINX2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[REFX]] : $*BitfieldOne
1337+
// CHECK-NEXT: end_access [[BEGINX2]] : $*BitfieldOne
13191338
// CHECK-NEXT: end_access [[BEGINY]] : $*Int32
13201339
// CHECK-LABEL: } // end sil function 'ref_elem_c'
13211340

@@ -1417,3 +1436,55 @@ bb6:
14171436
%19 = tuple ()
14181437
return %19 : $()
14191438
}
1439+
1440+
// public func testMergeWithFirstConflict() {
1441+
// Check that we can merge scopes even if the first one of them has conflicts within it
1442+
//
1443+
// CHECK-LABEL: sil @testMergeWithFirstConflict : $@convention(thin) () -> () {
1444+
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1445+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
1446+
// CHECK: apply
1447+
// CHECK-NEXT: load
1448+
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1449+
// CHECK-NOT: begin_access
1450+
// CHECK-LABEL: } // end sil function 'testMergeWithFirstConflict'
1451+
sil @testMergeWithFirstConflict : $@convention(thin) () -> () {
1452+
bb0:
1453+
%0 = global_addr @globalX: $*X
1454+
%4 = begin_access [read] [dynamic] %0 : $*X
1455+
%u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
1456+
%u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
1457+
end_access %4 : $*X
1458+
%1 = begin_access [read] [dynamic] %0 : $*X
1459+
%2 = load %1 : $*X
1460+
end_access %1 : $*X
1461+
%10 = tuple ()
1462+
return %10 : $()
1463+
}
1464+
1465+
// public func testMergeWithSecondConflict() {
1466+
// Check that we can merge scopes even if the 2nd one of them has conflicts within it
1467+
//
1468+
// CHECK-LABEL: sil @testMergeWithSecondConflict : $@convention(thin) () -> () {
1469+
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1470+
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
1471+
// CHECK: load
1472+
// CHECK: apply
1473+
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1474+
// CHECK-NOT: begin_access
1475+
// CHECK-LABEL: } // end sil function 'testMergeWithSecondConflict'
1476+
sil @testMergeWithSecondConflict : $@convention(thin) () -> () {
1477+
bb0:
1478+
%0 = global_addr @globalX: $*X
1479+
%1 = begin_access [read] [dynamic] %0 : $*X
1480+
%2 = load %1 : $*X
1481+
end_access %1 : $*X
1482+
%4 = begin_access [read] [dynamic] %0 : $*X
1483+
%u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
1484+
%u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
1485+
end_access %4 : $*X
1486+
%10 = tuple ()
1487+
return %10 : $()
1488+
}
1489+
1490+

0 commit comments

Comments
 (0)