Skip to content

Commit aa0833b

Browse files
eecksteineeckstein
authored
Merge pull request #79110 from eeckstein/verify_mark_dependence
SIL: Fix memory behavior of mark_dependence
2 parents 7bbca2c + 6a6d3f0 commit aa0833b

File tree

13 files changed

+164
-6
lines changed

13 files changed

+164
-6
lines changed

‎SwiftCompilerSources/Sources/Optimizer/Analysis/AliasAnalysis.swift‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,12 @@ struct AliasAnalysis {
253253
case let storeBorrow as StoreBorrowInst:
254254
return memLoc.mayAlias(with: storeBorrow.destination, self) ? .init(write: true) : .noEffects
255255

256+
case let mdi as MarkDependenceInst:
257+
if mdi.base.type.isAddress && memLoc.mayAlias(with: mdi.base, self) {
258+
return .init(read: true)
259+
}
260+
return .noEffects
261+
256262
case let copy as SourceDestAddrInstruction:
257263
let mayRead = memLoc.mayAlias(with: copy.source, self)
258264
let mayWrite = memLoc.mayAlias(with: copy.destination, self)

‎SwiftCompilerSources/Sources/Optimizer/TestPasses/MemBehaviorDumper.swift‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ private extension Instruction {
7474
is CopyAddrInst,
7575
is BuiltinInst,
7676
is StoreBorrowInst,
77+
is MarkDependenceInst,
7778
is DebugValueInst:
7879
return true
7980
default:

‎lib/SIL/IR/SILInstruction.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,6 +1081,12 @@ MemoryBehavior SILInstruction::getMemoryBehavior() const {
10811081
llvm_unreachable("Covered switch isn't covered?!");
10821082
}
10831083

1084+
if (auto *mdi = dyn_cast<MarkDependenceInst>(this)) {
1085+
if (mdi->getBase()->getType().isAddress())
1086+
return MemoryBehavior::MayRead;
1087+
return MemoryBehavior::None;
1088+
}
1089+
10841090
// TODO: An UncheckedTakeEnumDataAddr instruction has no memory behavior if
10851091
// it is nondestructive. Setting this currently causes LICM to miscompile
10861092
// because access paths do not account for enum projections.

‎lib/SIL/Utils/MemAccessUtils.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2829,6 +2829,7 @@ void swift::visitAccessedAddress(SILInstruction *I,
28292829
case SILInstructionKind::EndLifetimeInst:
28302830
case SILInstructionKind::ExistentialMetatypeInst:
28312831
case SILInstructionKind::FixLifetimeInst:
2832+
case SILInstructionKind::MarkDependenceInst:
28322833
case SILInstructionKind::GlobalAddrInst:
28332834
case SILInstructionKind::HasSymbolInst:
28342835
case SILInstructionKind::HopToExecutorInst:

‎lib/SIL/Utils/MemoryLocations.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,9 @@ static SILValue getBaseValue(SILValue addr) {
8989
case ValueKind::BeginAccessInst:
9090
addr = cast<BeginAccessInst>(addr)->getOperand();
9191
break;
92+
case ValueKind::MarkDependenceInst:
93+
addr = cast<MarkDependenceInst>(addr)->getValue();
94+
break;
9295
default:
9396
return addr;
9497
}
@@ -334,6 +337,14 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
334337
if (!cast<LoadBorrowInst>(user)->getUsersOfType<BranchInst>().empty())
335338
return false;
336339
break;
340+
case SILInstructionKind::MarkDependenceInst: {
341+
auto *mdi = cast<MarkDependenceInst>(user);
342+
if (use == &mdi->getAllOperands()[MarkDependenceInst::Value]) {
343+
if (!analyzeLocationUsesRecursively(mdi, locIdx, collectedVals, subLocationMap))
344+
return false;
345+
}
346+
break;
347+
}
337348
case SILInstructionKind::DebugValueInst:
338349
if (cast<DebugValueInst>(user)->hasAddrVal())
339350
break;

‎lib/SIL/Verifier/MemoryLifetimeVerifier.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,10 @@ void MemoryLifetimeVerifier::requireBitsSetForArgument(const Bits &bits, Operand
306306
}
307307

308308
bool MemoryLifetimeVerifier::applyMayRead(Operand *argOp, SILValue addr) {
309+
// Conservatively assume that a partial_apply does _not_ read an argument.
310+
if (isa<PartialApplyInst>(argOp->getUser()))
311+
return false;
312+
309313
FullApplySite as(argOp->getUser());
310314
CalleeList callees;
311315
if (calleeCache) {
@@ -881,6 +885,18 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
881885
locations.clearBits(bits, opVal);
882886
break;
883887
}
888+
case SILInstructionKind::MarkDependenceInst: {
889+
auto *mdi = cast<MarkDependenceInst>(&I);
890+
if (mdi->getBase()->getType().isAddress() &&
891+
// In case the mark_dependence is used for a closure it might be that the base
892+
// is "self" in an initializer and "self" is not fully initialized, yet.
893+
!mdi->getType().isFunction()) {
894+
requireBitsSet(bits, mdi->getBase(), &I);
895+
}
896+
// TODO: check that the base operand is alive during the whole lifetime
897+
// of the value operand.
898+
break;
899+
}
884900
default:
885901
break;
886902
}

‎lib/SILOptimizer/Transforms/TempRValueElimination.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,10 @@ collectLoads(Operand *addressUse, CopyAddrInst *originalCopy,
181181
}
182182
case SILInstructionKind::MarkDependenceInst: {
183183
auto mdi = cast<MarkDependenceInst>(user);
184-
// If the user is the base operand of the MarkDependenceInst we can return
185-
// true, because this would be the end of this dataflow chain
186184
if (mdi->getBase() == address) {
187-
return true;
185+
// We want to keep the original lifetime of the base. If we would eliminate
186+
// the base alloc_stack, we risk to insert a destroy_addr too early.
187+
return false;
188188
}
189189
// If the user is the value operand of the MarkDependenceInst we have to
190190
// transitively explore its uses until we reach a load or return false

‎test/SIL/memory_lifetime.sil‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,18 @@ bb0(%0 : $*T):
510510
return %res : $()
511511
}
512512

513+
sil [ossa] @test_non_escaping_closure_with_mark_dependence : $@convention(thin) () -> () {
514+
bb0:
515+
%0 = alloc_stack $T
516+
%func = function_ref @closure : $@convention(thin) (@in_guaranteed T) -> ()
517+
%pa = partial_apply [callee_guaranteed] [on_stack] %func(%0) : $@convention(thin) (@in_guaranteed T) -> ()
518+
%pa1 = mark_dependence %pa on %0
519+
destroy_value %pa1 : $@noescape @callee_guaranteed () -> ()
520+
dealloc_stack %0
521+
%res = tuple ()
522+
return %res : $()
523+
}
524+
513525
sil [ossa] @test_store_borrow : $@convention(thin) (@guaranteed T) -> () {
514526
bb0(%0 : @guaranteed $T):
515527
%s = alloc_stack $T
@@ -806,3 +818,35 @@ bb3:
806818
return %r : $()
807819
}
808820

821+
sil [ossa] @mark_dependence : $@convention(method) (@owned T, @owned Inner) -> @owned Inner {
822+
bb0(%0 : @owned $T, %1 : @owned $Inner):
823+
%2 = alloc_stack $T
824+
store %0 to [init] %2
825+
%4 = alloc_stack $Inner
826+
store %1 to [init] %4
827+
%6 = mark_dependence %4 on %2
828+
%7 = begin_access [read] [dynamic] %6
829+
%8 = load [take] %7
830+
end_access %7
831+
dealloc_stack %4
832+
destroy_addr %2
833+
dealloc_stack %2
834+
return %8
835+
}
836+
837+
sil [ossa] @mark_dependence_uninit_value : $@convention(thin) (@owned T, @owned Inner) -> @owned Inner {
838+
bb0(%0 : @owned $T, %1 : @owned $Inner):
839+
%2 = alloc_stack $T
840+
store %0 to [init] %2
841+
%4 = alloc_stack $Inner
842+
store %1 to [init] %4
843+
%6 = mark_dependence %4 on %2
844+
%7 = begin_access [read] [dynamic] %6
845+
%8 = load [take] %7
846+
end_access %7
847+
dealloc_stack %4
848+
destroy_addr %2
849+
dealloc_stack %2
850+
return %8
851+
}
852+

‎test/SIL/memory_lifetime_failures.sil‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,3 +792,20 @@ bb0(%0: $*Inner):
792792
return %r : $()
793793
}
794794

795+
// CHECK: SIL memory lifetime failure in @mark_dependence_uninit_base: memory is not initialized, but should be
796+
sil [ossa] @mark_dependence_uninit_base : $@convention(thin) (@owned T, @owned Inner) -> @owned Inner {
797+
bb0(%0 : @owned $T, %1 : @owned $Inner):
798+
%2 = alloc_stack $T
799+
store %0 to [init] %2
800+
%4 = alloc_stack $Inner
801+
store %1 to [init] %4
802+
destroy_addr %2
803+
%6 = mark_dependence %4 on %2
804+
%7 = begin_access [read] [dynamic] %6
805+
%8 = load [take] %7
806+
end_access %7
807+
dealloc_stack %4
808+
dealloc_stack %2
809+
return %8
810+
}
811+

‎test/SILOptimizer/mem-behavior.sil‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1851,3 +1851,32 @@ bb0(%0 : @owned $CL):
18511851
%3 = tuple ()
18521852
return %3
18531853
}
1854+
1855+
// CHECK-LABEL: @test_mark_dependence
1856+
// CHECK: PAIR #0.
1857+
// CHECK-NEXT: %3 = mark_dependence %1 : $*C on %2 : $C
1858+
// CHECK-NEXT: %0 = argument of bb0 : $*C
1859+
// CHECK-NEXT: r=0,w=0
1860+
// CHECK: PAIR #3.
1861+
// CHECK-NEXT: %4 = mark_dependence %2 : $C on %0 : $*C
1862+
// CHECK-NEXT: %0 = argument of bb0 : $*C
1863+
// CHECK-NEXT: r=1,w=0
1864+
// CHECK: PAIR #4.
1865+
// CHECK-NEXT: %4 = mark_dependence %2 : $C on %0 : $*C
1866+
// CHECK-NEXT: %1 = argument of bb0 : $*C
1867+
// CHECK-NEXT: r=0,w=0
1868+
// CHECK: PAIR #7.
1869+
// CHECK-NEXT: %5 = mark_dependence %1 : $*C on %0 : $*C
1870+
// CHECK-NEXT: %0 = argument of bb0 : $*C
1871+
// CHECK-NEXT: r=1,w=0
1872+
// CHECK: PAIR #8.
1873+
// CHECK-NEXT: %5 = mark_dependence %1 : $*C on %0 : $*C
1874+
// CHECK-NEXT: %1 = argument of bb0 : $*C
1875+
// CHECK-NEXT: r=0,w=0
1876+
sil [ossa] @test_mark_dependence : $@convention(thin) (@in_guaranteed C, @in_guaranteed C, @owned C) -> @owned C {
1877+
bb0(%0 : $*C, %1 : $*C, %2 : @owned $C):
1878+
%3 = mark_dependence %1 on %2
1879+
%4 = mark_dependence %2 on %0
1880+
%5 = mark_dependence %1 on %0
1881+
return %4
1882+
}

0 commit comments

Comments
 (0)