Skip to content

[Exclusivity] Merge scopes with nested conflicts #19372

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 1 commit into from
Sep 21, 2018
Merged

[Exclusivity] Merge scopes with nested conflicts #19372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 32 additions & 4 deletions lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,26 @@ void AccessConflictAndMergeAnalysis::visitBeginAccess(
// It can potentially be folded
// regardless of whether it may conflict with an outer access.
addInScopeAccess(info, beginAccess);
// We can merge out-of-scope regardless of having a conflict within a scope,
// normally, it would have made more sense to add it to out-of-scope set
// *only* after encountering the end_access instruction.
// However, that will lose us some valid optimization potential:
// consider the following pseudo-SIL:
// begin_access %x
// end_access %x
// begin_access %x
// conflict
// end_access %x
// we can merge both of these scopes
// but, if we only add the instr. after seeing end_access,
// then we would not have the first begin_access in out-of-scope
// set when encoutnering the 2nd end_access due to "conflict"
// NOTE: What we really want to do here is to check if
// we should add the new beginAccess to 'mergePairs' structure
// the reason for calling this method is to check for that.
// logically, we only need to add an instructio to
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typos. Can be fixed now or post-commit.

Otherwise seems ok.

// out-of-scope conflict-free set when we visit end_access
addOutOfScopeAccess(info, beginAccess);
}

void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
Expand All @@ -669,10 +689,18 @@ void AccessConflictAndMergeAnalysis::visitEndAccess(EndAccessInst *endAccess,
removeInScopeAccess(info, beginAccess);
}

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

Expand Down
107 changes: 89 additions & 18 deletions test/SILOptimizer/access_enforcement_opts.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,28 +48,17 @@ bb0(%0 : $@thin X.Type):
// }
// Preserve begin/end scope for nested conflicts,
// after inlining (read|modify)AndPerform.
// need to split it into 3 SIL functions else merging would kick in and we would be testing something else
//
// CHECK-LABEL: sil hidden @testNestedAccess : $@convention(thin) () -> () {
// CHECK-LABEL: sil hidden @testNestedAccess1 : $@convention(thin) () -> () {
// CHECK: [[F1:%.*]] = function_ref @testNestedAccessClosure1 : $@convention(thin) () -> ()
// CHECK: [[C1:%.*]] = convert_function [[F1]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
// CHECK: [[TF1:%.*]] = thin_to_thick_function [[C1]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed ()
// CHECK: [[A1:%.*]] = begin_access [read] [dynamic] %0 : $*X
// CHECK: apply [[TF1]]() : $@noescape @callee_guaranteed () -> ()
// CHECK: end_access [[A1]] : $*X
// CHECK: [[F2:%.*]] = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
// CHECK: [[C2:%.*]] = convert_function [[F2]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
// CHECK: [[TF2:%.*]] = thin_to_thick_function [[C2]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
// CHECK: [[A2:%.*]] = begin_access [modify] [dynamic] %0 : $*X
// CHECK: apply [[TF2]]() : $@noescape @callee_guaranteed () -> ()
// CHECK: end_access [[A2]] : $*X
// CHECK: [[F3:%.*]] = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
// CHECK: [[C3:%.*]] = convert_function [[F3]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
// CHECK: [[TF3:%.*]] = thin_to_thick_function [[C3]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
// CHECK: [[A3:%.*]] = begin_access [modify] [dynamic] %0 : $*X
// CHECK: apply [[TF3]]() : $@noescape @callee_guaranteed () -> ()
// CHECK: end_access [[A3]] : $*X
// CHECK-LABEL: } // end sil function 'testNestedAccess'
sil hidden @testNestedAccess : $@convention(thin) () -> () {
// CHECK-LABEL: } // end sil function 'testNestedAccess1'
sil hidden @testNestedAccess1 : $@convention(thin) () -> () {
bb0:
%2 = global_addr @globalX: $*X
%3 = function_ref @testNestedAccessClosure1 : $@convention(thin) () -> ()
Expand All @@ -78,14 +67,40 @@ bb0:
%6 = begin_access [read] [dynamic] %2 : $*X
%9 = apply %5() : $@noescape @callee_guaranteed () -> ()
end_access %6 : $*X

%36 = tuple ()
return %36 : $()
}
// CHECK-LABEL: sil hidden @testNestedAccess2 : $@convention(thin) () -> () {
// CHECK: [[F2:%.*]] = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
// CHECK: [[C2:%.*]] = convert_function [[F2]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
// CHECK: [[TF2:%.*]] = thin_to_thick_function [[C2]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
// CHECK: [[A2:%.*]] = begin_access [modify] [dynamic] %0 : $*X
// CHECK: apply [[TF2]]() : $@noescape @callee_guaranteed () -> ()
// CHECK: end_access [[A2]] : $*X
// CHECK-LABEL: } // end sil function 'testNestedAccess2'
sil hidden @testNestedAccess2 : $@convention(thin) () -> () {
bb0:
%2 = global_addr @globalX: $*X
%15 = function_ref @testNestedAccessClosure2 : $@convention(thin) () -> ()
%16 = convert_function %15 : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
%17 = thin_to_thick_function %16 : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
%18 = begin_access [modify] [dynamic] %2 : $*X
%21 = apply %17() : $@noescape @callee_guaranteed () -> ()
end_access %18 : $*X

%36 = tuple ()
return %36 : $()
}
// CHECK-LABEL: sil hidden @testNestedAccess3 : $@convention(thin) () -> () {
// CHECK: [[F3:%.*]] = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
// CHECK: [[C3:%.*]] = convert_function [[F3]] : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
// CHECK: [[TF3:%.*]] = thin_to_thick_function [[C3]] : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
// CHECK: [[A3:%.*]] = begin_access [modify] [dynamic] %0 : $*X
// CHECK: apply [[TF3]]() : $@noescape @callee_guaranteed () -> ()
// CHECK: end_access [[A3]] : $*X
// CHECK-LABEL: } // end sil function 'testNestedAccess3'
sil hidden @testNestedAccess3 : $@convention(thin) () -> () {
bb0:
%2 = global_addr @globalX: $*X
%27 = function_ref @testNestedAccessClosure3 : $@convention(thin) () -> ()
%28 = convert_function %27 : $@convention(thin) () -> () to $@convention(thin) @noescape () -> ()
%29 = thin_to_thick_function %28 : $@convention(thin) @noescape () -> () to $@noescape @callee_guaranteed () -> ()
Expand Down Expand Up @@ -1306,6 +1321,8 @@ class RefElemClass {
init()
}

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

Expand Down Expand Up @@ -1417,3 +1436,55 @@ bb6:
%19 = tuple ()
return %19 : $()
}

// public func testMergeWithFirstConflict() {
// Check that we can merge scopes even if the first one of them has conflicts within it
//
// CHECK-LABEL: sil @testMergeWithFirstConflict : $@convention(thin) () -> () {
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
// CHECK: apply
// CHECK-NEXT: load
// CHECK-NEXT: end_access [[BEGIN]] : $*X
// CHECK-NOT: begin_access
// CHECK-LABEL: } // end sil function 'testMergeWithFirstConflict'
sil @testMergeWithFirstConflict : $@convention(thin) () -> () {
bb0:
%0 = global_addr @globalX: $*X
%4 = begin_access [read] [dynamic] %0 : $*X
%u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
%u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
end_access %4 : $*X
%1 = begin_access [read] [dynamic] %0 : $*X
%2 = load %1 : $*X
end_access %1 : $*X
%10 = tuple ()
return %10 : $()
}

// public func testMergeWithSecondConflict() {
// Check that we can merge scopes even if the 2nd one of them has conflicts within it
//
// CHECK-LABEL: sil @testMergeWithSecondConflict : $@convention(thin) () -> () {
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
// CHECK: load
// CHECK: apply
// CHECK-NEXT: end_access [[BEGIN]] : $*X
// CHECK-NOT: begin_access
// CHECK-LABEL: } // end sil function 'testMergeWithSecondConflict'
sil @testMergeWithSecondConflict : $@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
%u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
%u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
end_access %4 : $*X
%10 = tuple ()
return %10 : $()
}