Skip to content

Commit 93dcb9b

Browse files
author
Joe Shajrawi
committed
[Exclusivity] Support exclusivity checks originating from block arguments
This includes: 1) Not crashing in AccessEnforcementOpts in case we have an Unidentified storage access - rdar://problem/45956642 and rdar://problem/45956777 2) Actually supporting finding the storage locations if we do begin_access <block argument> 3) Test case for said support
1 parent 0ccc525 commit 93dcb9b

File tree

3 files changed

+66
-6
lines changed

3 files changed

+66
-6
lines changed

lib/SIL/MemAccessUtils.cpp

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,31 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
287287
return AccessedStorage(address, AccessedStorage::Unidentified);
288288
return AccessedStorage();
289289

290-
// A block argument may be a box value projected out of
291-
// switch_enum. Address-type block arguments are not allowed.
292-
case ValueKind::SILPhiArgument:
290+
case ValueKind::SILPhiArgument: {
291+
auto *phiArg = cast<SILPhiArgument>(address);
292+
bool allValsMatch = true;
293+
SmallVector<SILValue, 8> incomingPhis;
294+
phiArg->getIncomingPhiValues(incomingPhis);
295+
if (!incomingPhis.empty()) {
296+
auto firstVal = incomingPhis.front();
297+
for (auto val : incomingPhis) {
298+
if (val != firstVal) {
299+
allValsMatch = false;
300+
break;
301+
}
302+
}
303+
if (allValsMatch) {
304+
return findAccessedStorage(firstVal);
305+
}
306+
}
307+
// A block argument may be a box value projected out of
308+
// switch_enum. Address-type block arguments are not allowed.
293309
if (address->getType().isAddress())
294310
return AccessedStorage();
295311

296312
checkSwitchEnumBlockArg(cast<SILPhiArgument>(address));
297313
return AccessedStorage(address, AccessedStorage::Unidentified);
298-
314+
}
299315
// Load a box from an indirect payload of an opaque enum.
300316
// We must have peeked past the project_box earlier in this loop.
301317
// (the indirectness makes it a box, the load is for address-only).

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,13 @@ class AccessConflictAndMergeAnalysis {
241241
/// Map each begin access to its AccessInfo with index, data, and flags.
242242
/// Iterating over this map is nondeterministic. If it is necessary to order
243243
/// the accesses, then AccessInfo::getAccessIndex() can be used.
244+
/// This maps contains every dynamic begin_access instruction,
245+
/// even those with invalid storage:
246+
/// We would like to keep track of unrecognized or invalid storage locations
247+
/// Because they affect our decisions for recognized locations,
248+
/// be it nested conflict or merging out of scope accesses.
249+
/// The access map is just a “cache” of accesses.
250+
/// Keeping those invalid ones just makes the lookup faster
244251
AccessMap accessMap;
245252

246253
/// Instruction pairs we can merge the scope of
@@ -536,8 +543,6 @@ void AccessConflictAndMergeAnalysis::identifyBeginAccesses() {
536543
// here as an invalid `storage` value.
537544
const AccessedStorage &storage =
538545
findAccessedStorageNonNested(beginAccess->getSource());
539-
if (!storage)
540-
continue;
541546

542547
auto iterAndSuccess = result.accessMap.try_emplace(
543548
beginAccess, static_cast<const AccessInfo &>(storage));

test/SILOptimizer/access_enforcement_opts.sil

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1515,4 +1515,43 @@ bb0:
15151515
return %10 : $()
15161516
}
15171517

1518+
// public func testPhiArgs() {
1519+
// Check that we can merge scopes with Phi args - avoiding a crash we've seen in the past
1520+
//
1521+
// CHECK-LABEL: sil @testPhiArgs : $@convention(thin) () -> () {
1522+
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
1523+
// CHECK: cond_br {{.*}}, bb1, bb2
1524+
// CHECK: bb1
1525+
// CHECK: br bb3([[GLOBAL]] : $*X)
1526+
// CHECK: bb2
1527+
// CHECK: br bb3([[GLOBAL]] : $*X)
1528+
// CHECK: bb3([[GLOBALPHI:%.*]] : $*X):
1529+
// CHECK: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBALPHI]] : $*X
1530+
// CHECK-NEXT: load
1531+
// CHECK-NEXT: load
1532+
// CHECK-NEXT: end_access [[BEGIN]] : $*X
1533+
// CHECK-NOT: begin_access
1534+
// CHECK-LABEL: } // end sil function 'testPhiArgs'
1535+
sil @testPhiArgs : $@convention(thin) () -> () {
1536+
bb0:
1537+
%0 = global_addr @globalX: $*X
1538+
%cond = integer_literal $Builtin.Int1, 1
1539+
cond_br %cond, bb1, bb2
1540+
1541+
bb1:
1542+
br bb3(%0 : $*X)
1543+
1544+
bb2:
1545+
br bb3(%0 : $*X)
1546+
1547+
bb3(%1 : $*X):
1548+
%2 = begin_access [read] [dynamic] %1 : $*X
1549+
%3 = load %2 : $*X
1550+
end_access %2 : $*X
1551+
%4 = begin_access [read] [dynamic] %0 : $*X
1552+
%5 = load %4 : $*X
1553+
end_access %4 : $*X
1554+
%10 = tuple ()
1555+
return %10 : $()
1556+
}
15181557

0 commit comments

Comments
 (0)