Skip to content

TempRValueOpt: extend access scopes #36540

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
Mar 23, 2021
Merged

TempRValueOpt: extend access scopes #36540

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
97 changes: 65 additions & 32 deletions lib/SILOptimizer/Transforms/TempRValueElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ class TempRValueOptPass : public SILFunctionTransform {
bool
checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst);

bool extendAccessScopes(CopyAddrInst *copyInst, SILInstruction *lastUseInst);

bool tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst);
std::pair<SILBasicBlock::iterator, bool>
tryOptimizeStoreIntoTemp(StoreInst *si);
Expand Down Expand Up @@ -167,12 +169,10 @@ collectLoads(Operand *addressUse, CopyAddrInst *originalCopy,
// %addr = begin_access [read]
// ... // there can be no writes to %addr here
// end_acess %addr // <- This is where the use actually ends.
for (Operand *accessUse : beginAccess->getUses()) {
if (auto *endAccess = dyn_cast<EndAccessInst>(accessUse->getUser())) {
if (endAccess->getParent() != block)
return false;
loadInsts.insert(endAccess);
}
for (EndAccessInst *endAccess : beginAccess->getEndAccesses()) {
if (endAccess->getParent() != block)
return false;
loadInsts.insert(endAccess);
}
return true;
}
Expand Down Expand Up @@ -347,6 +347,49 @@ SILInstruction *TempRValueOptPass::getLastUseWhileSourceIsNotModified(
return nullptr;
}

/// Tries to move an end_access down to extend the access scope over all uses
/// of the temporary. For example:
///
/// %a = begin_access %src
/// copy_addr %a to [initialization] %temp : $*T
/// end_access %a
/// use %temp
///
/// We must not replace %temp with %a after the end_access. Instead we try to
/// move the end_access after "use %temp".
bool TempRValueOptPass::extendAccessScopes(
CopyAddrInst *copyInst, SILInstruction *lastUseInst) {

SILValue copySrc = copyInst->getSrc();
EndAccessInst *endAccessToMove = nullptr;
auto begin = std::next(copyInst->getIterator());
auto end = std::next(lastUseInst->getIterator());

for (SILInstruction &inst : make_range(begin, end)) {
if (auto *endAccess = dyn_cast<EndAccessInst>(&inst)) {
// Is this the end of an access scope of the copy-source?
if (!aa->isNoAlias(copySrc, endAccess->getSource())) {
// To keep things simple, we can just move a single end_access.
if (endAccessToMove)
return false;
endAccessToMove = endAccess;
}
} else if (endAccessToMove) {
// We cannot move an end_access over a begin_access. This would destroy
// the proper nesting of accesses.
if (isa<BeginAccessInst>(&inst))
Copy link
Contributor

@atrick atrick Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@eeckstein We also still have an "unpaired" begin access, so strictly speaking, finding all the access scope beginnings means doing this:
isa<BeginAccessInst>(inst) || isa<BeginUnpairedAccessInst>(inst)
I'm surprised there's no helper for it yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The begin_unpaired_access is only used in the KeyPath runtime functions. And we compile the stdlib without access enforcement. So it should not be a problem.
But checking for the unpaired access does not harm. I'll add it.

return false;
// Don't extend a read-access scope over a (potential) write.
if (aa->mayWriteToMemory(&inst, endAccessToMove->getSource()))
return false;
}
}
if (endAccessToMove)
endAccessToMove->moveAfter(lastUseInst);

return true;
}

/// Return true if the \p tempObj, which is initialized by \p copyInst, is
/// destroyed in an orthodox way.
///
Expand Down Expand Up @@ -419,13 +462,7 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {

bool isOSSA = copyInst->getFunction()->hasOwnership();

// The copy's source address must not be a scoped instruction, like
// begin_borrow. When the temporary object is eliminated, it's uses are
// replaced with the copy's source. Therefore, the source address must be
// valid at least until the next instruction that may write to or destroy the
// source. End-of-scope markers, such as end_borrow, do not write to or
// destroy memory, so scoped addresses are not valid replacements.
SILValue copySrc = stripAccessMarkers(copyInst->getSrc());
SILValue copySrc = copyInst->getSrc();
assert(tempObj != copySrc && "can't initialize temporary with itself");

// If the source of the copyInst is taken, we must insert a compensating
Expand Down Expand Up @@ -487,6 +524,9 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
if (!isOSSA && !checkTempObjectDestroy(tempObj, copyInst))
return false;

if (!extendAccessScopes(copyInst, lastLoadInst))
return false;

LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj);

if (needToInsertDestroy) {
Expand Down Expand Up @@ -529,7 +569,7 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
auto *li = cast<LoadInst>(user);
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take)
li->setOwnershipQualifier(LoadOwnershipQualifier::Copy);
use->set(copyInst->getSrc());
use->set(copySrc);
break;
}

Expand Down Expand Up @@ -702,7 +742,7 @@ void TempRValueOptPass::run() {
bool changed = false;

// Find all copy_addr instructions.
llvm::SmallVector<CopyAddrInst *, 8> deadCopies;
llvm::SmallSetVector<CopyAddrInst *, 8> deadCopies;
for (auto &block : *getFunction()) {
// Increment the instruction iterator only after calling
// tryOptimizeCopyIntoTemp because the instruction after CopyInst might be
Expand All @@ -716,9 +756,9 @@ void TempRValueOptPass::run() {
// calling tryOptimizeCopyIntoTemp or was created by an earlier
// iteration, where another copy_addr copied the temporary back to the
// source location.
if (stripAccessMarkers(copyInst->getSrc()) == copyInst->getDest()) {
if (copyInst->getSrc() == copyInst->getDest()) {
changed = true;
deadCopies.push_back(copyInst);
deadCopies.insert(copyInst);
}
++ii;
continue;
Expand All @@ -735,21 +775,14 @@ void TempRValueOptPass::run() {
}
}

// Delete the copies and any unused address operands.
// The same copy may have been added multiple times.
sortUnique(deadCopies);
InstModCallbacks callbacks{
#ifndef NDEBUG
// With asserts, we include this assert. Otherwise, we use the default
// impl for perf.
[](SILInstruction *instToKill) {
// SimplifyInstruction is not in the business of removing
// copy_addr. If it were, then we would need to update deadCopies.
assert(!isa<CopyAddrInst>(instToKill));
instToKill->eraseFromParent();
}
#endif
};
InstModCallbacks callbacks(
[](SILInstruction *instToKill) {
// SimplifyInstruction is not in the business of removing
// copy_addr. If it were, then we would need to update deadCopies.
assert(!isa<CopyAddrInst>(instToKill));
instToKill->eraseFromParent();
}
);

DeadEndBlocks deBlocks(getFunction());
for (auto *deadCopy : deadCopies) {
Expand Down
15 changes: 10 additions & 5 deletions test/SILOptimizer/sil_combine_protocol_conf.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,13 +238,15 @@ public class OtherClass {
// CHECK: bb0([[ARG:%.*]] :
// CHECK: debug_value
// CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg1
// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*PropProtocol to $*@opened("{{.*}}") PropProtocol
// CHECK: [[ACC1:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R1]]
// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[ACC1]] : $*PropProtocol to $*@opened("{{.*}}") PropProtocol
// CHECK: [[U1:%.*]] = unchecked_addr_cast [[O1]] : $*@opened("{{.*}}") PropProtocol to $*PropClass
// CHECK: [[S1:%.*]] = struct_element_addr [[U1]] : $*PropClass, #PropClass.val
// CHECK: [[S11:%.*]] = struct_element_addr [[S1]] : $*Int, #Int._value
// CHECK: load [[S11]]
// CHECK: [[R2:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg2
// CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[R2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
// CHECK: [[ACC2:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R2]]
// CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[ACC2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
// CHECK: [[T1:%.*]] = alloc_stack $@opened
// CHECK: copy_addr [[O2]] to [initialization] [[T1]]
// CHECK: [[W2:%.*]] = witness_method $@opened("{{.*}}") GenericPropProtocol, #GenericPropProtocol.val!getter : <Self where Self : GenericPropProtocol> (Self) -> () -> Int, [[O2]] : $*@opened("{{.*}}") GenericPropProtocol : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
Expand All @@ -256,7 +258,8 @@ public class OtherClass {
// CHECK: tuple_extract
// CHECK: cond_fail
// CHECK: [[R4:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg3
// CHECK: [[O4:%.*]] = open_existential_addr immutable_access [[R4]] : $*NestedPropProtocol to $*@opened("{{.*}}") NestedPropProtocol
// CHECK: [[ACC4:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R4]]
// CHECK: [[O4:%.*]] = open_existential_addr immutable_access [[ACC4]] : $*NestedPropProtocol to $*@opened("{{.*}}") NestedPropProtocol
// CHECK: [[U4:%.*]] = unchecked_addr_cast [[O4]] : $*@opened("{{.*}}") NestedPropProtocol to $*Outer.Inner
// CHECK: [[S4:%.*]] = struct_element_addr [[U4]] : $*Outer.Inner, #Outer.Inner.val
// CHECK: [[S41:%.*]] = struct_element_addr [[S4]] : $*Int, #Int._value
Expand All @@ -266,7 +269,8 @@ public class OtherClass {
// CHECK: tuple_extract
// CHECK: cond_fail
// CHECK: [[R5:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg4
// CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[R5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
// CHECK: [[ACC5:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R5]]
// CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[ACC5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
// CHECK: [[T2:%.*]] = alloc_stack $@opened
// CHECK: copy_addr [[O5]] to [initialization] [[T2]]
// CHECK: [[W5:%.*]] = witness_method $@opened("{{.*}}") GenericNestedPropProtocol, #GenericNestedPropProtocol.val!getter : <Self where Self : GenericNestedPropProtocol> (Self) -> () -> Int, [[O5:%.*]] : $*@opened("{{.*}}") GenericNestedPropProtocol : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
Expand Down Expand Up @@ -336,7 +340,8 @@ public class OtherKlass {
// CHECK: debug_value
// CHECK: integer_literal
// CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherKlass, #OtherKlass.arg2
// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
// CHECK: [[ACC1:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R1]]
// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[ACC1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
// CHECK: [[T1:%.*]] = alloc_stack $@opened
// CHECK: copy_addr [[O1]] to [initialization] [[T1]]
// CHECK: [[W1:%.*]] = witness_method $@opened("{{.*}}") AGenericProtocol, #AGenericProtocol.val!getter : <Self where Self : AGenericProtocol> (Self) -> () -> Int, [[O1]] : $*@opened("{{.*}}") AGenericProtocol : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
Expand Down
63 changes: 55 additions & 8 deletions test/SILOptimizer/temp_rvalue_opt_ossa.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ bb0(%0 : $*Klass, %1 : $*Klass):
sil @throwing_function : $@convention(thin) (@in_guaranteed Klass) -> ((), @error Error)
sil @use_gsbase_builtinnativeobject : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()

sil_global @globalString : $String

///////////
// Tests //
///////////
Expand Down Expand Up @@ -830,16 +832,61 @@ bb3(%obj3 : @owned $GS<Builtin.NativeObject>):
return %obj3 : $GS<Builtin.NativeObject>
}

// Test copy elimination with access markers on both the source and dest.
//
// CHECK-LABEL: sil [ossa] @takeWithAccess : $@convention(thin) (@in Klass) -> () {
// CHECK-LABEL: sil [ossa] @extendAccessScopeOverLoad
// CHECK: [[GLOBAL:%.*]] = global_addr @globalString
// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
// CHECK: [[LOAD:%.*]] = load [copy] [[ACCESS]]
// CHECK: end_access [[ACCESS]]
// CHECK: return [[LOAD]]
// CHECK-LABEL: } // end sil function 'extendAccessScopeOverLoad'
sil [ossa] @extendAccessScopeOverLoad : $@convention(thin) () -> @owned String {
bb0:
%1 = global_addr @globalString : $*String
%3 = begin_access [read] [dynamic] %1 : $*String
%4 = alloc_stack $String
copy_addr %3 to [initialization] %4 : $*String
end_access %3 : $*String
%6 = load [copy] %4 : $*String
destroy_addr %4 : $*String
dealloc_stack %4 : $*String
return %6 : $String
}

sil [ossa] @loadString : $@convention(thin) (@in_guaranteed String) -> @owned String {
bb0(%0 : $*String):
%1 = load [copy] %0 : $*String
return %1 : $String
}

// CHECK-LABEL: sil [ossa] @extendAccessScopeOverApply
// CHECK: [[GLOBAL:%.*]] = global_addr @globalString
// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
// CHECK: [[RESULT:%.*]] = apply {{%[0-9]+}}([[ACCESS]])
// CHECK: end_access [[ACCESS]]
// CHECK: return [[RESULT]]
// CHECK-LABEL: } // end sil function 'extendAccessScopeOverApply'
sil [ossa] @extendAccessScopeOverApply : $@convention(thin) () -> @owned String {
bb0:
%1 = global_addr @globalString : $*String
%3 = begin_access [read] [dynamic] %1 : $*String
%4 = alloc_stack $String
copy_addr %3 to [initialization] %4 : $*String
end_access %3 : $*String
%f = function_ref @loadString : $@convention(thin) (@in_guaranteed String) -> @owned String
%a = apply %f(%4) : $@convention(thin) (@in_guaranteed String) -> @owned String
destroy_addr %4 : $*String
dealloc_stack %4 : $*String
return %a : $String
}

// CHECK-LABEL: sil [ossa] @dontExtendAccessScopeOverBeginAccess : $@convention(thin) (@in Klass) -> () {
// CHECK: bb0(%0 : $*Klass):
// CHECK: [[ACCESS:%.*]] = begin_access [read] [static] %0 : $*Klass
// CHECK: [[STACK:%.*]] = alloc_stack $Klass
// CHECK: [[ACCESS:%.*]] = begin_access [read] [static] [[STACK]] : $*Klass
// CHECK: apply %{{.*}}([[ACCESS]]) : $@convention(thin) (@in_guaranteed Klass) -> ()
// CHECK: end_access [[ACCESS]] : $*Klass
// CHECK: destroy_addr %0 : $*Klass
// CHECK-LABEL: } // end sil function 'takeWithAccess'
sil [ossa] @takeWithAccess : $@convention(thin) (@in Klass) -> () {
// CHECK: destroy_addr [[STACK]] : $*Klass
// CHECK-LABEL: } // end sil function 'dontExtendAccessScopeOverBeginAccess'
sil [ossa] @dontExtendAccessScopeOverBeginAccess : $@convention(thin) (@in Klass) -> () {
bb0(%0 : $*Klass):
%stack = alloc_stack $Klass
%access = begin_access [read] [static] %0 : $*Klass
Expand Down