Skip to content

DominatedAccessRemoval and AccessEnforcementOpts bug fixes #22929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 1, 2019
Merged

DominatedAccessRemoval and AccessEnforcementOpts bug fixes #22929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1dd7be
Merge pull request #22833 from shajrawi/access_opts_assert
Feb 23, 2019
c505dc5
Merge pull request #22838 from shajrawi/access_dom_nested
Feb 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/SILOptimizer/Transforms/AccessEnforcementDom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,17 @@ bool DominatedAccessRemoval::visitInstruction(
return true;
}

static EndAccessInst *getSingleEndAccess(BeginAccessInst *inst) {
EndAccessInst *end = nullptr;
for (auto *currEnd : inst->getEndAccesses()) {
if (end == nullptr)
end = currEnd;
else
return nullptr;
}
return end;
}

void DominatedAccessRemoval::visitBeginAccess(
BeginAccessInst *beginAccess, AccessedStorage storage,
AccessedStorageInfo &visitedDomAccessesToStorageInfo) {
Expand Down Expand Up @@ -185,6 +196,20 @@ void DominatedAccessRemoval::visitBeginAccess(
}

auto *parentBegin = visitedAccessesIt->first;

// If this beginAccess is a modify, and is nested inside parent
// then we have to bail: can't change it to static
// If, on the other hand, it is a read, we can continue.
if (beginAccess->getAccessKind() >= SILAccessKind::Modify) {
auto *endP = getSingleEndAccess(parentBegin);
if (!endP) {
return;
}
if (!domInfo->properlyDominates(endP, beginAccess)) {
return;
}
}

if (beginAccess->getAccessKind() > parentBegin->getAccessKind()) {
// we can't change to static: dominating access has a > access kind
// change parent's access kind so we would be able to do so
Expand Down
58 changes: 45 additions & 13 deletions lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,9 @@ class AccessConflictAndMergeAnalysis {
void addInScopeAccess(RegionInfo &info, BeginAccessInst *beginAccess);
void removeInScopeAccess(RegionInfo &info, BeginAccessInst *beginAccess);
void recordConflict(RegionInfo &info, const AccessedStorage &storage);
void addOutOfScopeAccess(RegionInfo &info, BeginAccessInst *beginAccess);
void addOutOfScopeAccessInsert(RegionInfo &info,
BeginAccessInst *beginAccess);
void addOutOfScopeAccessMerge(RegionInfo &info, BeginAccessInst *beginAccess);
void mergeAccessStruct(RegionInfo &info,
RegionInfo::AccessSummary &accessStruct,
const RegionInfo::AccessSummary &RHSAccessStruct);
Expand Down Expand Up @@ -386,7 +388,7 @@ void AccessConflictAndMergeAnalysis::recordConflict(
true /*isInScope*/);
}

void AccessConflictAndMergeAnalysis::addOutOfScopeAccess(
void AccessConflictAndMergeAnalysis::addOutOfScopeAccessInsert(
RegionInfo &info, BeginAccessInst *beginAccess) {
auto newStorageInfo = result.getAccessInfo(beginAccess);
auto pred = [&](BeginAccessInst *it) {
Expand All @@ -399,10 +401,42 @@ void AccessConflictAndMergeAnalysis::addOutOfScopeAccess(
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.rend(), pred);

if (it == info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.rend()) {
// We don't have a match in outOfScopeConflictFreeAccesses
// Just add it and return
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.insert(
beginAccess);
} else {
// we have a nested read case:
/*%4 = begin_access [read] [dynamic] %0 : $*X
%5 = load %4 : $*X
%7 = begin_access [read] [dynamic] %0 : $*X
%8 = load %7 : $*X
end_access %7 : $*X
end_access %4 : $*X*/
// we should remove the current one and insert the new.
auto *otherBegin = *it;
auto rmIt = std::find(
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.begin(),
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.end(),
otherBegin);
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.erase(rmIt);
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.insert(
beginAccess);
}
}

void AccessConflictAndMergeAnalysis::addOutOfScopeAccessMerge(
RegionInfo &info, BeginAccessInst *beginAccess) {
auto newStorageInfo = result.getAccessInfo(beginAccess);
auto pred = [&](BeginAccessInst *it) {
auto currStorageInfo = result.getAccessInfo(it);
return currStorageInfo.hasIdenticalBase(newStorageInfo);
};

auto it = std::find_if(
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.rbegin(),
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.rend(), pred);

if (it == info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.rend()) {
// We don't have a match in outOfScopeConflictFreeAccesses - return
return;
}

Expand All @@ -418,29 +452,27 @@ void AccessConflictAndMergeAnalysis::addOutOfScopeAccess(
return !currStorageInfo.isDistinctFrom(newStorageInfo);
};

auto itDistinct = std::find_if(
auto itNotDistinct = std::find_if(
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.begin(),
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.end(),
predDistinct);

if (itDistinct ==
if (itNotDistinct ==
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.end()) {
LLVM_DEBUG(llvm::dbgs() << "Found mergable pair: " << *otherBegin << ", "
<< *beginAccess << "\n");
result.mergePairs.push_back(std::make_pair(otherBegin, beginAccess));
} else {
while (itDistinct !=
while (itNotDistinct !=
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.end()) {
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.erase(
itDistinct);
itDistinct = std::find_if(
itNotDistinct);
itNotDistinct = std::find_if(
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.begin(),
info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.end(),
predDistinct);
}
}

info.outOfScopeConflictFreeAccesses.conflictFreeAccesses.insert(beginAccess);
}

void AccessConflictAndMergeAnalysis::mergeAccessStruct(
Expand Down Expand Up @@ -678,7 +710,7 @@ void AccessConflictAndMergeAnalysis::visitBeginAccess(
// the reason for calling this method is to check for that.
// logically, we only need to add an instructio to
// out-of-scope conflict-free set when we visit end_access
addOutOfScopeAccess(info, beginAccess);
addOutOfScopeAccessMerge(info, beginAccess);
}

void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
Expand Down Expand Up @@ -709,7 +741,7 @@ void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
LLVM_DEBUG(llvm::dbgs() << "Got out of scope from " << *beginAccess << " to "
<< *endAccess << "\n");

addOutOfScopeAccess(info, beginAccess);
addOutOfScopeAccessInsert(info, beginAccess);
}

void AccessConflictAndMergeAnalysis::detectApplyConflicts(
Expand Down
44 changes: 44 additions & 0 deletions test/SILOptimizer/access_dom.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,3 +540,47 @@ bb4:
%10 = tuple ()
return %10 : $()
}

// public func testBailOnNestedDomWrite() {
// Checks that we bail when the dominated begin comes before the dominating begin
//
// CHECK-LABEL: sil @testBailOnNestedDomWrite : $@convention(thin) () -> () {
// CHECK: bb0:
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
// CHECK: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-LABEL: } // end sil function 'testBailOnNestedDomWrite'
sil @testBailOnNestedDomWrite : $@convention(thin) () -> () {
bb0:
%0 = global_addr @globalX: $*X
%4 = begin_access [modify] [dynamic] %0 : $*X
%5 = load %4 : $*X
%7 = begin_access [modify] [dynamic] [no_nested_conflict] %0 : $*X
%8 = load %7 : $*X
end_access %7 : $*X
end_access %4 : $*X
%10 = tuple ()
return %10 : $()
}

// public func testOptOnNestedDomRead() {
// Checks that we bail when the dominated begin comes before the dominating begin
//
// CHECK-LABEL: sil @testOptOnNestedDomRead : $@convention(thin) () -> () {
// CHECK: bb0:
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
// CHECK: [[BEGIN2:%.*]] = begin_access [read] [static] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-LABEL: } // end sil function 'testOptOnNestedDomRead'
sil @testOptOnNestedDomRead : $@convention(thin) () -> () {
bb0:
%0 = global_addr @globalX: $*X
%4 = begin_access [read] [dynamic] %0 : $*X
%5 = load %4 : $*X
%7 = begin_access [read] [dynamic] [no_nested_conflict] %0 : $*X
%8 = load %7 : $*X
end_access %7 : $*X
end_access %4 : $*X
%10 = tuple ()
return %10 : $()
}
62 changes: 62 additions & 0 deletions test/SILOptimizer/access_enforcement_opts.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1650,3 +1650,65 @@ bb0(%0 : $TestClass):
%12 = tuple ()
return %12 : $()
}

// public func testNestedReadMerging() {
// Checks bailing on nested read scopes
// We will create two scopes - nested conflict
//
// CHECK-LABEL: sil @testNestedReadMerging : $@convention(thin) () -> () {
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
// CHECK-NEXT: load [[BEGIN]] : $*X
// CHECK-NEXT: load [[BEGIN]] : $*X
// CHECK-NEXT: [[BEGIN2:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-NEXT: load [[BEGIN2]] : $*X
// CHECK-NEXT: end_access [[BEGIN2]] : $*X
// CHECK-NEXT: end_access [[BEGIN]] : $*X
// CHECK-NOT: begin_access
// CHECK-LABEL: } // end sil function 'testNestedReadMerging'
sil @testNestedReadMerging : $@convention(thin) () -> () {
bb0:
%0 = global_addr @globalX: $*X
%1 = begin_access [read] [dynamic] %0 : $*X
%2 = load %1 : $*X
end_access %1 : $*X
%4 = begin_access [read] [dynamic] %0 : $*X
%5 = load %4 : $*X
%7 = begin_access [read] [dynamic] %0 : $*X
%8 = load %7 : $*X
end_access %7 : $*X
end_access %4 : $*X
%10 = tuple ()
return %10 : $()
}

// public func testNestedWriteBailout() {
// Same as testNestedReadBailout but with modify
// We will create two scopes - nested conflict
//
// CHECK-LABEL: sil @testNestedWriteBailout : $@convention(thin) () -> () {
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
// CHECK-NEXT: load [[BEGIN]] : $*X
// CHECK-NEXT: load [[BEGIN]] : $*X
// CHECK-NEXT: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-NEXT: load [[BEGIN2]] : $*X
// CHECK-NEXT: end_access [[BEGIN2]] : $*X
// CHECK-NEXT: end_access [[BEGIN]] : $*X
// CHECK-NOT: begin_access
// CHECK-LABEL: } // end sil function 'testNestedWriteBailout'
sil @testNestedWriteBailout : $@convention(thin) () -> () {
bb0:
%0 = global_addr @globalX: $*X
%1 = begin_access [modify] [dynamic] %0 : $*X
%2 = load %1 : $*X
end_access %1 : $*X
%4 = begin_access [modify] [dynamic] %0 : $*X
%5 = load %4 : $*X
%7 = begin_access [modify] [dynamic] %0 : $*X
%8 = load %7 : $*X
end_access %7 : $*X
end_access %4 : $*X
%10 = tuple ()
return %10 : $()
}