Skip to content

Simplify AccessEnforcementOpts by leaving access markers in place. #16835

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
May 29, 2018
Merged

Simplify AccessEnforcementOpts by leaving access markers in place. #16835

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
0aa2eae
AccessEnforcementOpts high-level comments.
atrick May 24, 2018
6280528
Simplify AccessEnforcementOpts by leaving access markers in place.
atrick May 25, 2018
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
68 changes: 44 additions & 24 deletions lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,19 @@
//
//===----------------------------------------------------------------------===//
///
/// Pass order dependencies:
///
/// - Will benefit from running after AccessEnforcementSelection.
///
/// - Should run immediately before the AccessEnforcementWMO to share
/// AccessedStorageAnalysis results.
///
/// This pass optimizes access enforcement as follows:
///
/// Access marker folding: Find begin/end access scopes that are uninterrupted
/// by a potential conflicting access. Flag those as [nontracking] access.
/// **Access marker folding**
///
/// Find begin/end access scopes that are uninterrupted by a potential
/// conflicting access. Flag those as [nontracking] access.
///
/// Folding must prove that no dynamic conflicts occur inside of an access
/// scope. That is, a scope has no "nested inner conflicts". The access itself
Expand All @@ -39,8 +48,15 @@
/// any path to an access' end of scope has a potentially conflicting access,
/// then that access is marked as a nested conflict.
///
/// Pass order dependencies:
/// - Will benefit from running after AccessEnforcementSelection.
/// **Local access marker removal**
///
/// When none of the local accesses on local storage (box/stack) have nested
/// conflicts, then all the local accesses may be disabled by setting their
/// enforcement to `static`. This is somwhat rare because static diagnostics
/// already promote the obvious cases to static checks. However, there are two
/// reasons that dynamic local markers may be disabled: (1) inlining may cause
/// closure access to become local access (2) local storage may truly escape,
/// but none of the the local access scopes cross a call site.
///
/// TODO: Perform another run of AccessEnforcementSelection immediately before
/// this pass. Currently, that pass only works well when run before
Expand Down Expand Up @@ -558,14 +574,25 @@ foldNonNestedAccesses(AccessConflictAnalysis::AccessMap &accessMap) {
return changed;
}

/// Eliminate accesses to uniquely identified local storage for which no
/// Perform local access marker elimination.
///
/// Disable accesses to uniquely identified local storage for which no
/// accesses can have nested conflicts. This is only valid if the function's
/// local storage cannot be potentially modified by unidentified access.
/// local storage cannot be potentially modified by unidentified access:
///
/// This simply invalidates the AccessMap result rather than erasing individual
/// entries.
/// - Arguments cannot alias with local storage, so accessing an argument has no
/// effect on analysis of the current function. When a callee accesses an
/// argument, AccessedStorageAnalysis will either map the accessed storage to
/// a value in the caller's function, or mark it as unidentified.
///
/// - Stack or Box local storage could potentially be accessed via Unidentified
/// access. (Some Unidentified accesses are for initialization or for
/// temporary storage instead, but those should never have Dynamic
/// enforcement). These accesses can only be eliminated when there is no
/// Unidentified access within the function without the [no_nested_conflict]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording in the last sentence here is a little bit confusing. had to re-read it a couple of times to understand. could be easier to understand if an example is show. or if it is expanded to explain how the presence of a [no_nested_conflict] - less access prevents us from eliminating accesses.
Otherwise LGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks. I changed it to simply read:
/// - Stack or Box local storage could potentially be accessed via Unidentified
/// access. (Some Unidentified accesses are for initialization or for
/// temporary storage instead, but those should always have Unsafe
/// enforcement). If this function contains an Unidentified access without its
/// [no_nested_conflict] flag set, then any of the function's local storage
/// could have a nested conflict.

There are comments elsewhere in the code related to it.

/// flag.
static bool
removeLocalNonNestedAccess(AccessConflictAnalysis::Result &&result,
removeLocalNonNestedAccess(const AccessConflictAnalysis::Result &result,
const FunctionAccessedStorage &functionAccess) {
if (functionAccess.hasUnidentifiedAccess())
return false;
Expand All @@ -574,24 +601,18 @@ removeLocalNonNestedAccess(AccessConflictAnalysis::Result &&result,
SmallVector<BeginAccessInst *, 8> deadAccesses;
for (auto &beginAccessAndInfo : result.accessMap) {
BeginAccessInst *beginAccess = beginAccessAndInfo.first;
AccessInfo &info = beginAccessAndInfo.second;
const AccessInfo &info = beginAccessAndInfo.second;
if (info.seenNestedConflict() || !info.isLocal())
continue;

// This particular access to local storage is marked
// [no_nested_conflict]. Now check FunctionAccessedStorage to determine if
// that is true for all access to the same storage.
if (functionAccess.hasNoNestedConflict(info))
deadAccesses.push_back(beginAccess);
}
std::sort(deadAccesses.begin(), deadAccesses.end(),
[&result](BeginAccessInst *a, BeginAccessInst *b) {
return result.getAccessIndex(a) < result.getAccessIndex(b);
});
for (BeginAccessInst *beginAccess : deadAccesses) {
DEBUG(llvm::dbgs() << "Removing dead access " << *beginAccess);
changed = true;
removeBeginAccess(beginAccess);
if (functionAccess.hasNoNestedConflict(info)) {
DEBUG(llvm::dbgs() << "Disabling dead access " << *beginAccess);
beginAccess->setEnforcement(SILAccessEnforcement::Static);
changed = true;
}
}
return changed;
}
Expand All @@ -618,15 +639,14 @@ struct AccessEnforcementOpts : public SILFunctionTransform {

// Use the updated AccessedStorageAnalysis to find any uniquely identified
// local storage that has no nested conflict on any of its accesses within
// this function. These can be removed entirely.
// this function. All the accesses can be marked as statically enforced.
//
// Note that the storage address may be passed as an argument and there may
// be nested conflicts within that call, but none of the accesses within
// this function will overlap.
const FunctionAccessedStorage &functionAccess = ASA->getEffects(F);
if (removeLocalNonNestedAccess(std::move(result), functionAccess))
if (removeLocalNonNestedAccess(result, functionAccess))
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
// `result` is now invalid.
}
};
} // namespace
Expand Down
11 changes: 8 additions & 3 deletions test/SILOptimizer/access_enforcement_opts.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ bb0:
// Demote both read accesses to non_nested, then remove the local outer access.
//
// CHECK-LABEL: sil @testCaptureReadRead : $@convention(thin) () -> () {
// CHECK-NOT: begin_access
// CHECK: begin_access [read] [static] [no_nested_conflict] %0 : $*X
// CHECK-LABEL: } // end sil function 'testCaptureReadRead'
sil @testCaptureReadRead : $@convention(thin) () -> () {
bb0:
Expand Down Expand Up @@ -246,7 +246,9 @@ bb0(%0 : $*X):
// Demote both read accesses to non_nested, and remove the local outer access.
//
// CHECK-LABEL: sil @testCaptureBoxReadRead : $@convention(thin) () -> () {
// CHECK-NOT: begin_access
// CHECK: [[BOX:%.*]] = alloc_box ${ var X }, var, name "x"
// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var X }, 0
// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*X
// CHECK-LABEL: } // end sil function 'testCaptureBoxReadRead'
sil @testCaptureBoxReadRead : $@convention(thin) () -> () {
bb0:
Expand Down Expand Up @@ -361,7 +363,10 @@ bb0(%0 : $*Int64):
// Demote read/read access to [no_nested_conflict].
//
// CHECK-LABEL: sil @testInoutReadEscapeRead : $@convention(thin) () -> () {
// CHECK-NOT: begin_access
// CHECK: [[BOX:%.*]] = alloc_box ${ var Int64 }, var, name "x"
// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var Int64 }, 0
// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*Int64
// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*Int64
// CHECK-LABEL: } // end sil function 'testInoutReadEscapeRead'
sil @testInoutReadEscapeRead : $@convention(thin) () -> () {
bb0:
Expand Down