Skip to content

Commit ad9e090

Browse files
nate-chandlernate-chandler
committed
[SIL] Only visit final on-stack pai lifetime ends.
In OSSA, the `partial_apply [on_stack]` instruction produces a value with odd characteristics that correspond to the fact that it is lowered to a stack-allocating instruction. Among these characteristics is the fact that copies of such values aren't load bearing. When visiting the lifetime-ending uses of a `partial_apply [on_stack]` the lifetime ending uses of (transitive) copies of the partial_apply must be considered as well. Otherwise, the borrow scope it defins may be incorrectly short: ``` %closure = partial_apply %fn(%value) // borrows %value %closure2 = copy_value %closure destroy_value %closure // does _not_ end borrow of %value! ... destroy_value %closure2 // ends borrow of %value ... destroy_value %value ``` Furthermore, _only_ the final such destroys actually count as the real lifetime ends. At least one client (OME) relies on `visitOnStackLifetimeEnds` visiting at most a single lifetime end on any path. Rewrite the utility to use PrunedLiveness, tracking only destroys of copies and forwards. The final destroys are the destroys on the boundary. rdar://142636711
1 parent f9ddf8d commit ad9e090

File tree

3 files changed

+148
-34
lines changed

3 files changed

+148
-34
lines changed

‎lib/SIL/IR/SILInstruction.cpp‎

Lines changed: 104 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@
1515
//===----------------------------------------------------------------------===//
1616

1717
#include "swift/SIL/SILInstruction.h"
18-
#include "swift/Basic/Assertions.h"
1918
#include "swift/Basic/AssertImplements.h"
19+
#include "swift/Basic/Assertions.h"
2020
#include "swift/Basic/Unicode.h"
2121
#include "swift/Basic/type_traits.h"
2222
#include "swift/SIL/ApplySite.h"
2323
#include "swift/SIL/DynamicCasts.h"
24+
#include "swift/SIL/InstWrappers.h"
2425
#include "swift/SIL/InstructionUtils.h"
26+
#include "swift/SIL/NodeDatastructures.h"
2527
#include "swift/SIL/OwnershipUtils.h"
28+
#include "swift/SIL/PrunedLiveness.h"
2629
#include "swift/SIL/SILBuilder.h"
2730
#include "swift/SIL/SILCloner.h"
2831
#include "swift/SIL/SILDebugScope.h"
@@ -1869,6 +1872,38 @@ visitRecursivelyLifetimeEndingUses(
18691872
return true;
18701873
}
18711874

1875+
static SILValue lookThroughOwnershipAndForwardingInsts(SILValue value) {
1876+
auto current = value;
1877+
while (true) {
1878+
if (auto *inst = current->getDefiningInstruction()) {
1879+
switch (inst->getKind()) {
1880+
case SILInstructionKind::MoveValueInst:
1881+
case SILInstructionKind::CopyValueInst:
1882+
case SILInstructionKind::BeginBorrowInst:
1883+
current = inst->getOperand(0);
1884+
continue;
1885+
default:
1886+
break;
1887+
}
1888+
auto forward = ForwardingOperation(inst);
1889+
Operand *op = nullptr;
1890+
if (forward && (op = forward.getSingleForwardingOperand())) {
1891+
current = op->get();
1892+
continue;
1893+
}
1894+
} else if (auto *result = SILArgument::isTerminatorResult(current)) {
1895+
auto *op = result->forwardedTerminatorResultOperand();
1896+
if (!op) {
1897+
break;
1898+
}
1899+
current = op->get();
1900+
continue;
1901+
}
1902+
break;
1903+
}
1904+
return current;
1905+
}
1906+
18721907
bool
18731908
PartialApplyInst::visitOnStackLifetimeEnds(
18741909
llvm::function_ref<bool (Operand *)> func) const {
@@ -1877,29 +1912,74 @@ PartialApplyInst::visitOnStackLifetimeEnds(
18771912
&& "only meaningful for OSSA stack closures");
18781913
bool noUsers = true;
18791914

1880-
auto visitUnknownUse = [](Operand *unknownUse) {
1881-
// There shouldn't be any dead-end consumptions of a nonescaping
1882-
// partial_apply that don't forward it along, aside from destroy_value.
1883-
//
1884-
// On-stack partial_apply cannot be cloned, so it should never be used by a
1885-
// BranchInst.
1886-
//
1887-
// This is a fatal error because it performs SIL verification that is not
1888-
// separately checked in the verifier. It is the only check that verifies
1889-
// the structural requirements of on-stack partial_apply uses.
1890-
llvm::errs() << "partial_apply [on_stack] use:\n";
1891-
auto *user = unknownUse->getUser();
1892-
user->printInContext(llvm::errs());
1893-
if (isa<BranchInst>(user)) {
1894-
llvm::report_fatal_error("partial_apply [on_stack] cannot be cloned");
1895-
}
1896-
llvm::report_fatal_error("partial_apply [on_stack] must be directly "
1897-
"forwarded to a destroy_value");
1898-
return false;
1899-
};
1900-
if (!visitRecursivelyLifetimeEndingUses(this, noUsers, func,
1901-
visitUnknownUse)) {
1902-
return false;
1915+
auto *function = getFunction();
1916+
1917+
SmallVector<SILBasicBlock *, 32> discoveredBlocks;
1918+
SSAPrunedLiveness liveness(function, &discoveredBlocks);
1919+
liveness.initializeDef(this);
1920+
1921+
StackList<SILValue> values(function);
1922+
values.push_back(this);
1923+
1924+
while (!values.empty()) {
1925+
auto value = values.pop_back_val();
1926+
for (auto *use : value->getUses()) {
1927+
if (!use->isConsuming()) {
1928+
if (auto *cvi = dyn_cast<CopyValueInst>(use->getUser())) {
1929+
values.push_back(cvi);
1930+
}
1931+
continue;
1932+
}
1933+
noUsers = false;
1934+
if (isa<DestroyValueInst>(use->getUser())) {
1935+
liveness.updateForUse(use->getUser(), /*lifetimeEnding=*/true);
1936+
continue;
1937+
}
1938+
auto forward = ForwardingOperand(use);
1939+
if (!forward) {
1940+
// There shouldn't be any non-forwarding consumptions of a nonescaping
1941+
// partial_apply that don't forward it along, aside from destroy_value.
1942+
//
1943+
// On-stack partial_apply cannot be cloned, so it should never be used
1944+
// by a BranchInst.
1945+
//
1946+
// This is a fatal error because it performs SIL verification that is
1947+
// not separately checked in the verifier. It is the only check that
1948+
// verifies the structural requirements of on-stack partial_apply uses.
1949+
if (lookThroughOwnershipAndForwardingInsts(use->get()) !=
1950+
SILValue(this)) {
1951+
// Consumes of values which aren't "essentially" the
1952+
// partial_apply [on_stack]
1953+
// are okay. For example, a not-on_stack partial_apply that captures
1954+
// it.
1955+
continue;
1956+
}
1957+
llvm::errs() << "partial_apply [on_stack] use:\n";
1958+
auto *user = use->getUser();
1959+
user->printInContext(llvm::errs());
1960+
if (isa<BranchInst>(user)) {
1961+
llvm::report_fatal_error("partial_apply [on_stack] cannot be cloned");
1962+
}
1963+
llvm::report_fatal_error("partial_apply [on_stack] must be directly "
1964+
"forwarded to a destroy_value");
1965+
}
1966+
forward.visitForwardedValues([&values](auto value) {
1967+
values.push_back(value);
1968+
return true;
1969+
});
1970+
}
1971+
}
1972+
PrunedLivenessBoundary boundary;
1973+
liveness.computeBoundary(boundary);
1974+
1975+
for (auto *inst : boundary.lastUsers) {
1976+
// Only destroy_values were added to liveness, so only destroy_values can be
1977+
// the last users.
1978+
auto *dvi = cast<DestroyValueInst>(inst);
1979+
auto keepGoing = func(&dvi->getOperandRef());
1980+
if (!keepGoing) {
1981+
return false;
1982+
}
19031983
}
19041984
return !noUsers;
19051985
}

‎test/SIL/Instruction/partial_apply_on_stack_lifetime_ends.sil‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ sil @borrowC : $@convention(thin) (@guaranteed C) -> ()
2121
// CHECK: destroy_value [[C]]
2222
// CHECK-LABEL: } // end sil function 'copied_partial_apply'
2323
// CHECK: Operand.
24-
// CHECK: Owner: destroy_value [[PA]]
24+
// CHECK: Owner: destroy_value [[PA2]]
2525
// CHECK: returned: true
2626
// CHECK-LABEL: end running test {{.*}} on copied_partial_apply
2727
sil [ossa] @copied_partial_apply : $@convention(thin) (@owned C) -> () {

‎test/SILOptimizer/canonicalize_ossa_lifetime_unit.sil‎

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,9 @@ exit(%phi : @owned $C, %typhi : $S):
162162
sil @empty : $@convention(thin) () -> () {
163163
[global: ]
164164
bb0:
165-
%0 = tuple ()
166-
return %0 : $()
167-
}
165+
%0 = tuple ()
166+
return %0 : $()
167+
}
168168

169169
// Even though the apply of %empty is not a deinit barrier, verify that the
170170
// destroy is not hoisted, because MoS is move-only.
@@ -568,16 +568,16 @@ entry(%c1 : @owned $C):
568568
// CHECK: bb0([[C1:%[^,]+]] : @owned $C):
569569
// CHECK: [[TAKE_C:%[^,]+]] = function_ref @takeC
570570
// CHECK: [[BARRIER:%[^,]+]] = function_ref @barrier
571-
// CHECK: cond_br undef, [[LEFT]], [[RIGHT]]
572-
// CHECK: [[LEFT]]:
571+
// CHECK: cond_br undef, [[LEFT]], [[RIGHT]]
572+
// CHECK: [[LEFT]]:
573573
// CHECK: [[M:%[^,]+]] = move_value [[C1]]
574574
// CHECK: apply [[TAKE_C]]([[M]])
575-
// CHECK: br [[EXIT]]
576-
// CHECK: [[RIGHT]]:
575+
// CHECK: br [[EXIT]]
576+
// CHECK: [[RIGHT]]:
577577
// CHECK: apply [[BARRIER]]()
578578
// CHECK: destroy_value [[C1]]
579-
// CHECK: br [[EXIT]]
580-
// CHECK: [[EXIT]]:
579+
// CHECK: br [[EXIT]]
580+
// CHECK: [[EXIT]]:
581581
// CHECK: apply [[BARRIER]]()
582582
// CHECK-LABEL: } // end sil function 'lexical_end_at_end_2'
583583
// CHECK-LABEL: end running test {{.*}} on lexical_end_at_end_2: canonicalize_ossa_lifetime
@@ -884,3 +884,37 @@ die:
884884
apply undef(%reload) : $@convention(thin) (@guaranteed C) -> ()
885885
unreachable
886886
}
887+
888+
// The destroy of a value must not be hoisted over a destroy of a copy of a
889+
// partial_apply [on_stack] which captures the value.
890+
// CHECK-LABEL: begin running test {{.*}} on destroy_after_pa_copy_destroy
891+
// CHECK-LABEL: sil [ossa] @destroy_after_pa_copy_destroy : {{.*}} {
892+
// CHECK: bb0([[A:%[^,]+]] :
893+
// CHECK: [[C:%[^,]+]] = move_value [[A]]
894+
// CHECK: [[BORROW_C:%[^,]+]] = function_ref @borrowC
895+
// CHECK: [[PA:%[^,]+]] = partial_apply [callee_guaranteed] [on_stack] [[BORROW_C]]([[C]])
896+
// CHECK: [[PA2:%[^,]+]] = copy_value [[PA]]
897+
// CHECK: destroy_value [[PA]]
898+
// CHECK: destroy_value [[PA2]]
899+
// CHECK: destroy_value [[C]]
900+
// CHECK: apply undef()
901+
// CHECK-LABEL: } // end sil function 'destroy_after_pa_copy_destroy'
902+
// CHECK-LABEL: end running test {{.*}} on destroy_after_pa_copy_destroy
903+
sil [ossa] @destroy_after_pa_copy_destroy : $@convention(thin) (@owned C) -> () {
904+
entry(%a: @owned $C):
905+
// To defeat deinit barriers (apply undef).
906+
%c = move_value %a
907+
specify_test "canonicalize_ossa_lifetime true false true %c"
908+
909+
%callee = function_ref @borrowC : $@convention(thin) (@guaranteed C) -> ()
910+
%pa = partial_apply [callee_guaranteed] [on_stack] %callee(%c) : $@convention(thin) (@guaranteed C) -> ()
911+
%pa2 = copy_value %pa
912+
destroy_value %pa
913+
914+
destroy_value %pa2
915+
// To defeat ignoredByDestroyHoisting.
916+
apply undef() : $@convention(thin) () -> ()
917+
destroy_value %c
918+
%retval = tuple ()
919+
return %retval
920+
}

0 commit comments

Comments
 (0)