From 2f102022693f16e2262687de80818d94d8fb2446 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Mon, 20 Apr 2020 16:05:09 -0700 Subject: [PATCH] [semantic-arc-opts] Use all consuming uses instead of just destroying uses when validating if a LiveRange is alive in a scope. The reason why this is important is that if our destroy_value is elided due to the destroy_value being in a dead end block, we can promote a load [copy] to a load_borrow even if the load [copy] has a forwarding consuming use outside of a begin_access region. I changed every place in SemanticARCOpts that did this sort of thing to use this pattern instead of just destroys so that no one cargo cults the original pattern by mistake. (cherry picked from commit ba1ac7875805483e8430a5650742dd71c41d8d5b) Cherry-pick radar: rdar://63188362 --- .../Transforms/SemanticARCOpts.cpp | 91 ++++++++++++++----- test/SILOptimizer/semantic-arc-opts.sil | 29 ++++++ 2 files changed, 95 insertions(+), 25 deletions(-) diff --git a/lib/SILOptimizer/Transforms/SemanticARCOpts.cpp b/lib/SILOptimizer/Transforms/SemanticARCOpts.cpp index c40ec0b6a30fb..1b4c5204a70be 100644 --- a/lib/SILOptimizer/Transforms/SemanticARCOpts.cpp +++ b/lib/SILOptimizer/Transforms/SemanticARCOpts.cpp @@ -52,8 +52,23 @@ class LiveRange { /// introducer and not to be forwarding. OwnedValueIntroducer introducer; + /// A vector that we store all of our uses into. + /// + /// Some properties of this array are: + /// + /// 1. It is only mutated in the constructor of LiveRange. + /// + /// 2. destroyingUses, ownershipForwardingUses, and unknownConsumingUses are + /// views into this array. We store the respective uses in the aforementioned + /// order. This is why it is important not to mutate consumingUses after we + /// construct the LiveRange since going from small -> large could invalidate + /// the uses. + SmallVector consumingUses; + /// A list of destroy_values of the live range. - SmallVector destroyingUses; + /// + /// This is just a view into consuming uses. + ArrayRef destroyingUses; /// A list of forwarding instructions that forward owned ownership, but that /// are also able to be converted to guaranteed ownership. If we are able to @@ -61,12 +76,12 @@ class LiveRange { /// flip the ownership of all of these instructions to guaranteed from owned. /// /// Corresponds to isOwnershipForwardingInst(...). - SmallVector ownershipForwardingUses; + ArrayRef ownershipForwardingUses; /// Consuming uses that we were not able to understand as a forwarding /// instruction or a destroy_value. These must be passed a strongly control /// equivalent +1 value. - SmallVector unknownConsumingUses; + ArrayRef unknownConsumingUses; public: LiveRange(SILValue value); @@ -87,6 +102,11 @@ class LiveRange { HasConsumingUse_t hasUnknownConsumingUse(bool assumingFixedPoint = false) const; + /// Return an array ref to /all/ consuming uses. Will include all 3 sorts of + /// consuming uses: destroying uses, forwarding consuming uses, and unknown + /// forwarding instruction. + ArrayRef getAllConsumingUses() const { return consumingUses; } + ArrayRef getDestroyingUses() const { return destroyingUses; } private: @@ -121,7 +141,7 @@ class LiveRange { return ownershipForwardingUses; } - void convertOwnedGeneralForwardingUsesToGuaranteed(); + void convertOwnedGeneralForwardingUsesToGuaranteed() &&; /// A consuming operation that: /// @@ -192,6 +212,10 @@ LiveRange::LiveRange(SILValue value) ownershipForwardingUses(), unknownConsumingUses() { assert(introducer.value.getOwnershipKind() == ValueOwnershipKind::Owned); + SmallVector tmpDestroyingUses; + SmallVector tmpForwardingConsumingUses; + SmallVector tmpUnknownConsumingUses; + // We know that our silvalue produces an @owned value. Look through all of our // uses and classify them as either consuming or not. SmallVector worklist(introducer.value->getUses()); @@ -220,7 +244,7 @@ LiveRange::LiveRange(SILValue value) // Ok, we know now that we have a consuming use. See if we have a destroy // value, quickly up front. If we do have one, stash it and continue. if (isa(user)) { - destroyingUses.push_back(op); + tmpDestroyingUses.push_back(op); continue; } @@ -244,13 +268,13 @@ LiveRange::LiveRange(SILValue value) return v.getOwnershipKind() == ValueOwnershipKind::Owned; })) { - unknownConsumingUses.push_back(op); + tmpUnknownConsumingUses.push_back(op); continue; } // Ok, this is a forwarding instruction whose ownership we can flip from // owned -> guaranteed. - ownershipForwardingUses.push_back(op); + tmpForwardingConsumingUses.push_back(op); // If we have a non-terminator, just visit its users recursively to see if // the the users force the live range to be alive. @@ -284,6 +308,20 @@ LiveRange::LiveRange(SILValue value) } } } + + // The order in which we append these to consumingUses matters since we assume + // their order as an invariant. This is done to ensure that we can pass off + // all of our uses or individual sub-arrays of our users without needing to + // move around memory. + llvm::copy(tmpDestroyingUses, std::back_inserter(consumingUses)); + llvm::copy(tmpForwardingConsumingUses, std::back_inserter(consumingUses)); + llvm::copy(tmpUnknownConsumingUses, std::back_inserter(consumingUses)); + + auto cUseArrayRef = llvm::makeArrayRef(consumingUses); + destroyingUses = cUseArrayRef.take_front(tmpDestroyingUses.size()); + ownershipForwardingUses = cUseArrayRef.slice( + tmpDestroyingUses.size(), tmpForwardingConsumingUses.size()); + unknownConsumingUses = cUseArrayRef.take_back(tmpUnknownConsumingUses.size()); } void LiveRange::insertEndBorrowsAtDestroys( @@ -339,9 +377,10 @@ void LiveRange::insertEndBorrowsAtDestroys( } } -void LiveRange::convertOwnedGeneralForwardingUsesToGuaranteed() { +void LiveRange::convertOwnedGeneralForwardingUsesToGuaranteed() && { while (!ownershipForwardingUses.empty()) { - auto *i = ownershipForwardingUses.pop_back_val()->getUser(); + auto *i = ownershipForwardingUses.back()->getUser(); + ownershipForwardingUses = ownershipForwardingUses.drop_back(); // If this is a term inst, just convert all of its incoming values that are // owned to be guaranteed. @@ -402,7 +441,8 @@ void LiveRange::convertToGuaranteedAndRAUW(SILValue newGuaranteedValue, InstModCallbacks callbacks) && { auto *value = cast(introducer.value); while (!destroyingUses.empty()) { - auto *d = destroyingUses.pop_back_val(); + auto *d = destroyingUses.back(); + destroyingUses = destroyingUses.drop_back(); callbacks.deleteInst(d->getUser()); ++NumEliminatedInsts; } @@ -412,7 +452,7 @@ void LiveRange::convertToGuaranteedAndRAUW(SILValue newGuaranteedValue, // Then change all of our guaranteed forwarding insts to have guaranteed // ownership kind instead of what ever they previously had (ignoring trivial // results); - convertOwnedGeneralForwardingUsesToGuaranteed(); + std::move(*this).convertOwnedGeneralForwardingUsesToGuaranteed(); } void LiveRange::convertArgToGuaranteed(DeadEndBlocks &deadEndBlocks, @@ -429,7 +469,8 @@ void LiveRange::convertArgToGuaranteed(DeadEndBlocks &deadEndBlocks, // Then eliminate all of the destroys... while (!destroyingUses.empty()) { - auto *d = destroyingUses.pop_back_val(); + auto *d = destroyingUses.back(); + destroyingUses = destroyingUses.drop_back(); callbacks.deleteInst(d->getUser()); ++NumEliminatedInsts; } @@ -437,7 +478,7 @@ void LiveRange::convertArgToGuaranteed(DeadEndBlocks &deadEndBlocks, // and change all of our guaranteed forwarding insts to have guaranteed // ownership kind instead of what ever they previously had (ignoring trivial // results); - convertOwnedGeneralForwardingUsesToGuaranteed(); + std::move(*this).convertOwnedGeneralForwardingUsesToGuaranteed(); } LiveRange::HasConsumingUse_t @@ -1297,8 +1338,9 @@ bool SemanticARCOptVisitor::performGuaranteedCopyValueOptimization(CopyValueInst SmallVector scratchSpace; SmallPtrSet visitedBlocks; if (llvm::any_of(borrowScopeIntroducers, [&](BorrowedValue borrowScope) { - return !borrowScope.areUsesWithinScope( - destroys, scratchSpace, visitedBlocks, getDeadEndBlocks()); + return !borrowScope.areUsesWithinScope(lr.getAllConsumingUses(), + scratchSpace, visitedBlocks, + getDeadEndBlocks()); })) { return false; } @@ -1330,12 +1372,11 @@ bool SemanticARCOptVisitor::performGuaranteedCopyValueOptimization(CopyValueInst return false; } - if (llvm::any_of(borrowScopeIntroducers, - [&](BorrowedValue borrowScope) { - return !borrowScope.areUsesWithinScope( - phiArgLR.getDestroyingUses(), scratchSpace, - visitedBlocks, getDeadEndBlocks()); - })) { + if (llvm::any_of(borrowScopeIntroducers, [&](BorrowedValue borrowScope) { + return !borrowScope.areUsesWithinScope( + phiArgLR.getAllConsumingUses(), scratchSpace, visitedBlocks, + getDeadEndBlocks()); + })) { return false; } } @@ -1641,7 +1682,7 @@ class StorageGuaranteesLoadVisitor SmallPtrSet visitedBlocks; LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks()); if (!checker.validateLifetime(access, endScopeUses, - liveRange.getDestroyingUses())) { + liveRange.getAllConsumingUses())) { // If we fail the linear lifetime check, then just recur: return next(access->getOperand()); } @@ -1738,8 +1779,8 @@ class StorageGuaranteesLoadVisitor LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks()); // Returns true on success. So we invert. - bool foundError = !checker.validateLifetime(baseObject, endScopeInsts, - liveRange.getDestroyingUses()); + bool foundError = !checker.validateLifetime( + baseObject, endScopeInsts, liveRange.getAllConsumingUses()); return answer(foundError); } @@ -1777,7 +1818,7 @@ class StorageGuaranteesLoadVisitor // Returns true on success. So we invert. bool foundError = !checker.validateLifetime( stack, destroyAddrOperands /*consuming users*/, - liveRange.getDestroyingUses() /*non consuming users*/); + liveRange.getAllConsumingUses() /*non consuming users*/); return answer(foundError); } diff --git a/test/SILOptimizer/semantic-arc-opts.sil b/test/SILOptimizer/semantic-arc-opts.sil index 93f565b65ef20..30d2ed520f47f 100644 --- a/test/SILOptimizer/semantic-arc-opts.sil +++ b/test/SILOptimizer/semantic-arc-opts.sil @@ -59,6 +59,7 @@ class ClassLet { @_hasStorage let aLet: Klass @_hasStorage var aVar: Klass @_hasStorage let aLetTuple: (Klass, Klass) + @_hasStorage let anOptionalLet: FakeOptional @_hasStorage let anotherLet: ClassLet } @@ -2204,3 +2205,31 @@ bb6: %9999 = tuple() return %9999 : $() } + +// Make sure that we do not promote the load [copy] to a load_borrow since it +// has a use outside of the access scope. +// +// CHECK-LABEL: sil [ossa] @deadEndBlockDoNotPromote : $@convention(method) (@guaranteed ClassLet) -> () { +// CHECK: load_borrow +// CHECK: load [copy] +// CHECK: } // end sil function 'deadEndBlockDoNotPromote' +sil [ossa] @deadEndBlockDoNotPromote : $@convention(method) (@guaranteed ClassLet) -> () { +bb0(%0 : @guaranteed $ClassLet): + %4 = ref_element_addr %0 : $ClassLet, #ClassLet.anotherLet + %5 = load [copy] %4 : $*ClassLet + %6 = begin_borrow %5 : $ClassLet + %7 = ref_element_addr %6 : $ClassLet, #ClassLet.anOptionalLet + %8 = begin_access [read] [dynamic] %7 : $*FakeOptional + %9 = load [copy] %8 : $*FakeOptional + end_access %8 : $*FakeOptional + end_borrow %6 : $ClassLet + destroy_value %5 : $ClassLet + switch_enum %9 : $FakeOptional, case #FakeOptional.none!enumelt: bb1, case #FakeOptional.some!enumelt: bb2 + +bb1: + %107 = tuple () + return %107 : $() + +bb2(%39 : @owned $Klass): + unreachable +}