Skip to content

Commit 9da5a4b

Browse files
gottesmmgottesmm
committed
[move-function-addr] Fix a subtle bug where we were not inserting debug info in the case of vars.
The specific problem was that we assume that SILGen will only emit a single debug_value for a value. This condition breaks once we start processing since we are inserting extra debug_value when reiniting. The end result is that we do not insert the undef, addr. To fix this I just hoisted out this computation upon the address before we do anything.
1 parent e1c4e10 commit 9da5a4b

File tree

2 files changed

+70
-47
lines changed

2 files changed

+70
-47
lines changed

‎lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp

Lines changed: 59 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,13 +1399,14 @@ struct DataflowState {
13991399
closureConsumes(closureConsumes) {}
14001400
void init();
14011401
bool process(
1402-
SILValue address,
1402+
SILValue address, DebugVarCarryingInst addressDebugInst,
14031403
SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
14041404
bool handleSingleBlockClosure(SILArgument *address,
14051405
ClosureOperandState &state);
14061406
bool cleanupAllDestroyAddr(
1407-
SILValue address, SILFunction *fn, SmallBitVector &destroyIndices,
1408-
SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
1407+
SILValue address, DebugVarCarryingInst addressDebugInst, SILFunction *fn,
1408+
SmallBitVector &destroyIndices, SmallBitVector &reinitIndices,
1409+
SmallBitVector &consumingClosureIndices,
14091410
BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
14101411
BasicBlockSet &blocksWithMovesThatAreNowTakes,
14111412
SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers);
@@ -1423,16 +1424,15 @@ struct DataflowState {
14231424
} // namespace
14241425

14251426
bool DataflowState::cleanupAllDestroyAddr(
1426-
SILValue address, SILFunction *fn, SmallBitVector &destroyIndices,
1427-
SmallBitVector &reinitIndices, SmallBitVector &consumingClosureIndices,
1427+
SILValue address, DebugVarCarryingInst addressDebugInst, SILFunction *fn,
1428+
SmallBitVector &destroyIndices, SmallBitVector &reinitIndices,
1429+
SmallBitVector &consumingClosureIndices,
14281430
BasicBlockSet &blocksVisitedWhenProcessingNewTakes,
14291431
BasicBlockSet &blocksWithMovesThatAreNowTakes,
14301432
SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
14311433
bool madeChange = false;
14321434
BasicBlockWorklist worklist(fn);
14331435

1434-
auto debugVarInst = DebugVarCarryingInst::getFromValue(address);
1435-
14361436
LLVM_DEBUG(llvm::dbgs() << "Cleanup up destroy addr!\n");
14371437
LLVM_DEBUG(llvm::dbgs() << " Visiting destroys!\n");
14381438
LLVM_DEBUG(llvm::dbgs() << " Destroy Indices: " << destroyIndices << "\n");
@@ -1537,12 +1537,13 @@ bool DataflowState::cleanupAllDestroyAddr(
15371537
convertMemoryReinitToInitForm(*reinit);
15381538

15391539
// Make sure to create a new debug_value for the reinit value.
1540-
if (debugVarInst) {
1541-
if (auto varInfo = debugVarInst.getVarInfo()) {
1540+
if (addressDebugInst) {
1541+
if (auto varInfo = addressDebugInst.getVarInfo()) {
15421542
SILBuilderWithScope reinitBuilder(*reinit);
1543-
reinitBuilder.setCurrentDebugScope(debugVarInst->getDebugScope());
1544-
reinitBuilder.createDebugValue(debugVarInst.inst->getLoc(), address,
1545-
*varInfo, false, /*was moved*/ true);
1543+
reinitBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
1544+
reinitBuilder.createDebugValue(
1545+
addressDebugInst.inst->getLoc(), address, *varInfo, false,
1546+
/*was moved*/ true);
15461547
}
15471548
}
15481549
madeChange = true;
@@ -1583,7 +1584,7 @@ bool DataflowState::cleanupAllDestroyAddr(
15831584
}
15841585

15851586
bool DataflowState::process(
1586-
SILValue address,
1587+
SILValue address, DebugVarCarryingInst addressDebugInst,
15871588
SmallBlotSetVector<SILInstruction *, 8> &postDominatingConsumingUsers) {
15881589
SILFunction *fn = address->getFunction();
15891590
assert(fn);
@@ -1612,6 +1613,7 @@ bool DataflowState::process(
16121613
BasicBlockSet blocksVisitedWhenProcessingNewTakes(fn);
16131614
BasicBlockSet blocksWithMovesThatAreNowTakes(fn);
16141615
bool convertedMarkMoveToTake = false;
1616+
16151617
for (auto *mvi : markMovesThatPropagateDownwards) {
16161618
bool emittedSingleDiagnostic = false;
16171619

@@ -1777,13 +1779,13 @@ bool DataflowState::process(
17771779

17781780
// Now that we have processed all of our mark_moves, eliminate all of the
17791781
// destroy_addr and set our debug value as being moved.
1780-
if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
1781-
debug.markAsMoved();
1782-
if (auto varInfo = debug.getVarInfo()) {
1782+
if (addressDebugInst) {
1783+
addressDebugInst.markAsMoved();
1784+
if (auto varInfo = addressDebugInst.getVarInfo()) {
17831785
SILBuilderWithScope undefBuilder(builder);
1784-
undefBuilder.setCurrentDebugScope(debug->getDebugScope());
1786+
undefBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
17851787
undefBuilder.createDebugValue(
1786-
debug->getLoc(),
1788+
addressDebugInst->getLoc(),
17871789
SILUndef::get(address->getType(), builder.getModule()), *varInfo,
17881790
false /*poison*/, true /*was moved*/);
17891791
}
@@ -1806,8 +1808,8 @@ bool DataflowState::process(
18061808
// Now that we have processed all of our mark_moves, eliminate all of the
18071809
// destroy_addr.
18081810
madeChange |= cleanupAllDestroyAddr(
1809-
address, fn, getIndicesOfPairedDestroys(), getIndicesOfPairedReinits(),
1810-
getIndicesOfPairedConsumingClosureUses(),
1811+
address, addressDebugInst, fn, getIndicesOfPairedDestroys(),
1812+
getIndicesOfPairedReinits(), getIndicesOfPairedConsumingClosureUses(),
18111813
blocksVisitedWhenProcessingNewTakes, blocksWithMovesThatAreNowTakes,
18121814
postDominatingConsumingUsers);
18131815

@@ -1932,6 +1934,7 @@ struct MoveKillsCopyableAddressesChecker {
19321934
void emitDiagnosticForMove(SILValue borrowedValue,
19331935
StringRef borrowedValueName, MoveValueInst *mvi);
19341936
bool performSingleBasicBlockAnalysis(SILValue address,
1937+
DebugVarCarryingInst addressDebugInst,
19351938
MarkUnresolvedMoveAddrInst *mvi);
19361939

19371940
ASTContext &getASTContext() const { return fn->getASTContext(); }
@@ -2049,7 +2052,8 @@ bool MoveKillsCopyableAddressesChecker::performClosureDataflow(
20492052
// case. Returns false if we visited all of the uses and seeded the UseState
20502053
// struct with the information needed to perform our interprocedural dataflow.
20512054
bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
2052-
SILValue address, MarkUnresolvedMoveAddrInst *mvi) {
2055+
SILValue address, DebugVarCarryingInst addressDebugInst,
2056+
MarkUnresolvedMoveAddrInst *mvi) {
20532057
// First scan downwards to make sure we are move out of this block.
20542058
auto &useState = dataflowState.useState;
20552059
auto &applySiteToPromotedArgIndices =
@@ -2075,17 +2079,17 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
20752079
builder.createCopyAddr(mvi->getLoc(), mvi->getSrc(), mvi->getDest(), IsTake,
20762080
IsInitialization);
20772081
// Also, mark the alloc_stack as being moved at some point.
2078-
if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
2079-
if (auto varInfo = debug.getVarInfo()) {
2082+
if (addressDebugInst) {
2083+
if (auto varInfo = addressDebugInst.getVarInfo()) {
20802084
SILBuilderWithScope undefBuilder(builder);
2081-
undefBuilder.setCurrentDebugScope(debug->getDebugScope());
2085+
undefBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
20822086
undefBuilder.createDebugValue(
2083-
debug->getLoc(),
2087+
addressDebugInst->getLoc(),
20842088
SILUndef::get(address->getType(), builder.getModule()), *varInfo,
20852089
false,
20862090
/*was moved*/ true);
20872091
}
2088-
debug.markAsMoved();
2092+
addressDebugInst.markAsMoved();
20892093
}
20902094

20912095
useState.destroys.erase(dvi);
@@ -2185,29 +2189,29 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
21852189
SILBuilderWithScope builder(mvi);
21862190
builder.createCopyAddr(mvi->getLoc(), mvi->getSrc(), mvi->getDest(), IsTake,
21872191
IsInitialization);
2188-
if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
2189-
if (auto varInfo = debug.getVarInfo()) {
2192+
if (addressDebugInst) {
2193+
if (auto varInfo = addressDebugInst.getVarInfo()) {
21902194
{
21912195
SILBuilderWithScope undefBuilder(builder);
2192-
undefBuilder.setCurrentDebugScope(debug->getDebugScope());
2196+
undefBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
21932197
undefBuilder.createDebugValue(
2194-
debug->getLoc(),
2198+
addressDebugInst->getLoc(),
21952199
SILUndef::get(address->getType(), builder.getModule()), *varInfo,
21962200
false,
2197-
/*was moved*/ true);
2201+
/*was moved*/ true);
21982202
}
21992203
{
22002204
// Make sure at the reinit point to create a new debug value after the
22012205
// reinit instruction so we reshow the variable.
22022206
auto *next = interestingUser->getNextInstruction();
22032207
SILBuilderWithScope reinitBuilder(next);
2204-
reinitBuilder.setCurrentDebugScope(debug->getDebugScope());
2205-
reinitBuilder.createDebugValue(debug->getLoc(), address, *varInfo,
2206-
false,
2208+
reinitBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
2209+
reinitBuilder.createDebugValue(addressDebugInst->getLoc(),
2210+
address, *varInfo, false,
22072211
/*was moved*/ true);
22082212
}
22092213
}
2210-
debug.markAsMoved();
2214+
addressDebugInst.markAsMoved();
22112215
}
22122216
mvi->eraseFromParent();
22132217
return false;
@@ -2238,17 +2242,17 @@ bool MoveKillsCopyableAddressesChecker::performSingleBasicBlockAnalysis(
22382242
LLVM_DEBUG(llvm::dbgs() << "Found apply site to clone: " << **fas);
22392243
LLVM_DEBUG(llvm::dbgs() << "BitVector: ";
22402244
dumpBitVector(llvm::dbgs(), bitVector); llvm::dbgs() << '\n');
2241-
if (auto debug = DebugVarCarryingInst::getFromValue(address)) {
2242-
if (auto varInfo = debug.getVarInfo()) {
2245+
if (addressDebugInst) {
2246+
if (auto varInfo = addressDebugInst.getVarInfo()) {
22432247
SILBuilderWithScope undefBuilder(builder);
2244-
undefBuilder.setCurrentDebugScope(debug->getDebugScope());
2248+
undefBuilder.setCurrentDebugScope(addressDebugInst->getDebugScope());
22452249
undefBuilder.createDebugValue(
2246-
debug->getLoc(),
2250+
addressDebugInst->getLoc(),
22472251
SILUndef::get(address->getType(), builder.getModule()), *varInfo,
22482252
false,
22492253
/*was moved*/ true);
22502254
}
2251-
debug.markAsMoved();
2255+
addressDebugInst.markAsMoved();
22522256
}
22532257
mvi->eraseFromParent();
22542258
return false;
@@ -2337,9 +2341,21 @@ bool MoveKillsCopyableAddressesChecker::check(SILValue address) {
23372341
// routine also prepares the pass for running the multi-basic block
23382342
// diagnostic.
23392343
bool emittedSingleBBDiagnostic = false;
2344+
2345+
// Before we process any moves, gather the debug inst associated with our
2346+
// address.
2347+
//
2348+
// NOTE: The reason why we do this early is that we rely on our address
2349+
// initially having a single DebugValueCarryingInst (either an alloc_stack
2350+
// itself or a debug_value associated with an argument). If we do this while
2351+
// processing, as we insert additional debug info we will cause this condition
2352+
// to begin failing.
2353+
auto addressDebugInst = DebugVarCarryingInst::getFromValue(address);
2354+
23402355
for (auto *mvi : useState.markMoves) {
23412356
LLVM_DEBUG(llvm::dbgs() << "Performing single block analysis on: " << *mvi);
2342-
emittedSingleBBDiagnostic |= performSingleBasicBlockAnalysis(address, mvi);
2357+
emittedSingleBBDiagnostic |=
2358+
performSingleBasicBlockAnalysis(address, addressDebugInst, mvi);
23432359
}
23442360

23452361
if (emittedSingleBBDiagnostic) {
@@ -2359,7 +2375,8 @@ bool MoveKillsCopyableAddressesChecker::check(SILValue address) {
23592375
// Ok, we need to perform global dataflow for one of our moves. Initialize our
23602376
// dataflow state engine and then run the dataflow itself.
23612377
dataflowState.init();
2362-
bool result = dataflowState.process(address, closureConsumes);
2378+
bool result = dataflowState.process(
2379+
address, addressDebugInst, closureConsumes);
23632380
return result;
23642381
}
23652382

‎test/DebugInfo/move_function_dbginfo_async.swift

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,13 @@ public func forceSplit() async {}
4747

4848
// CHECK-LABEL: define internal swifttailcc void @"$s27move_function_dbginfo_async13letSimpleTestyyxnYalFTQ0_"(i8* swiftasync %0)
4949
// CHECK: entryresume.0:
50-
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[SIMPLE_TEST_METADATA_2:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 24, DW_OP_plus_uconst, 8, DW_OP_deref)),
50+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[SIMPLE_TEST_METADATA_2:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref)),
5151
// CHECK: musttail call swifttailcc void
5252
// CHECK-NEXT: ret void
5353
//
5454
// CHECK-LABEL: define internal swifttailcc void @"$s27move_function_dbginfo_async13letSimpleTestyyxnYalFTY1_"(i8* swiftasync %0)
5555
// CHECK: entryresume.1:
56-
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[SIMPLE_TEST_METADATA_3:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 24, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC:[0-9]+]]
56+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[SIMPLE_TEST_METADATA_3:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC:[0-9]+]]
5757
// CHECK: call void @llvm.dbg.value(metadata %swift.opaque* undef, metadata ![[SIMPLE_TEST_METADATA_3]], metadata !DIExpression(DW_OP_deref)), !dbg ![[ADDR_LOC]]
5858
// CHECK: musttail call swifttailcc void
5959
// CHECK-NEXT: ret void
@@ -90,14 +90,14 @@ public func letSimpleTest<T>(_ msg: __owned T) async {
9090
//
9191
// CHECK-LABEL: define internal swifttailcc void @"$s27move_function_dbginfo_async13varSimpleTestyyxz_xtYalFTQ0_"(i8* swiftasync %0)
9292
// CHECK: entryresume.0:
93-
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata !{{[0-9]+}}, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 24, DW_OP_plus_uconst, 8, DW_OP_deref))
93+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata !{{[0-9]+}}, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref))
9494
// CHECK: musttail call swifttailcc void @swift_task_switch(%swift.context* swiftasync %9, i8* bitcast (void (i8*)* @"$s27move_function_dbginfo_async13varSimpleTestyyxz_xtYalFTY1_" to i8*), i64 0, i64 0)
9595
// CHECK-NEXT: ret void
9696
// CHECK-NEXT: }
9797
//
9898
// CHECK-LABEL: define internal swifttailcc void @"$s27move_function_dbginfo_async13varSimpleTestyyxz_xtYalFTY1_"(i8* swiftasync %0)
9999
// CHECK: entryresume.1:
100-
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[METADATA:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 24, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC:[0-9]+]]
100+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[METADATA:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC:[0-9]+]]
101101
// CHECK: call void @llvm.dbg.value(metadata %swift.opaque* undef, metadata ![[METADATA]], metadata !DIExpression(DW_OP_deref)), !dbg ![[ADDR_LOC]]
102102
// CHECK: musttail call swifttailcc void @"$s27move_function_dbginfo_async10forceSplityyYaF"(%swift.context* swiftasync %34)
103103
// CHECK-NEXT: ret void
@@ -108,7 +108,12 @@ public func letSimpleTest<T>(_ msg: __owned T) async {
108108

109109
// CHECK-LABEL: define internal swifttailcc void @"$s27move_function_dbginfo_async13varSimpleTestyyxz_xtYalFTY3_"(i8* swiftasync %0)
110110
// CHECK: entryresume.3:
111-
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata !{{[0-9]+}}, metadata !DIExpression(DW_OP_plus_uconst, 24, DW_OP_plus_uconst, 8, DW_OP_deref))
111+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[METADATA:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC:[0-9]+]]
112+
// CHECK: call void @llvm.dbg.value(metadata %swift.opaque* undef, metadata ![[METADATA]], metadata !DIExpression(DW_OP_deref)), !dbg ![[ADDR_LOC]]
113+
// CHECK: call void @llvm.dbg.addr(metadata i8* %0, metadata ![[METADATA]], metadata !DIExpression(DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 8, DW_OP_deref)), !dbg ![[ADDR_LOC]]
114+
// CHECK: musttail call swifttailcc void @"$s27move_function_dbginfo_async10forceSplityyYaF"(
115+
// CHECK-NEXT: ret void
116+
// CHECK-NEXT: }
112117
//
113118
// DWARF: DW_AT_linkage_name ("$s3out13varSimpleTestyyxz_xtYalF")
114119
// DWARF: DW_AT_name ("varSimpleTest")
@@ -159,6 +164,7 @@ public func letSimpleTest<T>(_ msg: __owned T) async {
159164
// DWARF: DW_TAG_formal_parameter
160165
// DWARF-NEXT: DW_AT_location (DW_OP_entry_value(DW_OP_reg14 R14), DW_OP_plus_uconst 0x[[MSG_LOC]], DW_OP_plus_uconst 0x8, DW_OP_deref)
161166
// DWARF-NEXT: DW_AT_name ("msg")
167+
162168
public func varSimpleTest<T>(_ msg: inout T, _ msg2: T) async {
163169
await forceSplit()
164170
use(_move(msg))

0 commit comments

Comments
 (0)