Skip to content

Commit 48546fd

Browse files
gottesmmgottesmm
committed
[move-only] Move BorrowToDestructureTransform back into the Object Checker rather than as a separate pass.
The reason that I am doing this is I discovered that we emit worse quality diagnostics since if we emit an error while running the borrow to destructure transform, we want to do a complete cleanup to be safe (e.x.: converting copy_value -> explicit_copy_value). This causes the object checker to then not emit any additional diagnostics for other variables that were not impacted by the BorrowToDestructureTransform, reducing the quality of diagnostics in a significant way that isn't needed.
1 parent 39d3264 commit 48546fd

File tree

6 files changed

+293
-168
lines changed

6 files changed

+293
-168
lines changed

‎lib/SILOptimizer/Mandatory/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ target_sources(swiftSILOptimizer PRIVATE
2626
MovedAsyncVarDebugInfoPropagator.cpp
2727
MoveOnlyAddressChecker.cpp
2828
MoveOnlyBorrowToDestructureTransform.cpp
29+
MoveOnlyBorrowToDestructureTransformTester.cpp
2930
MoveOnlyDeinitInsertion.cpp
3031
MoveOnlyDiagnostics.cpp
3132
MoveOnlyObjectChecker.cpp

‎lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransform.cpp

Lines changed: 10 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,16 @@
1010
//
1111
//===----------------------------------------------------------------------===//
1212
///
13-
/// \file This is a pass that converts the borrow + gep pattern to destructures
14-
/// or emits an error if it cannot be done. It is assumed that it runs
15-
/// immediately before move checking of objects runs. This ensures that the move
16-
/// checker does not need to worry about this problem and instead can just check
17-
/// that the newly inserted destructures do not cause move only errors.
13+
/// \file This is a transform that converts the borrow + gep pattern to
14+
/// destructures or emits an error if it cannot be done. It is assumed that it
15+
/// runs immediately before move checking of objects runs. This ensures that the
16+
/// move checker does not need to worry about this problem and instead can just
17+
/// check that the newly inserted destructures do not cause move only errors.
18+
///
19+
/// This is written as a utility so that we can have a utility pass that tests
20+
/// this directly but also invoke this via the move only object checker.
21+
///
22+
/// TODO: Move this to SILOptimizer/Utils.
1823
///
1924
//===----------------------------------------------------------------------===//
2025

@@ -1322,146 +1327,3 @@ void BorrowToDestructureTransform::cleanup(
13221327
// And finally do the same thing for our initial copy_value.
13231328
addCompensatingDestroys(liveness, boundary, initialValue);
13241329
}
1325-
1326-
//===----------------------------------------------------------------------===//
1327-
// Top Level Entrypoint
1328-
//===----------------------------------------------------------------------===//
1329-
1330-
static bool runTransform(SILFunction *fn,
1331-
ArrayRef<MarkMustCheckInst *> moveIntroducersToProcess,
1332-
PostOrderAnalysis *poa,
1333-
DiagnosticEmitter &diagnosticEmitter) {
1334-
BorrowToDestructureTransform::IntervalMapAllocator allocator;
1335-
bool madeChange = false;
1336-
while (!moveIntroducersToProcess.empty()) {
1337-
auto *mmci = moveIntroducersToProcess.back();
1338-
moveIntroducersToProcess = moveIntroducersToProcess.drop_back();
1339-
1340-
unsigned currentDiagnosticCount = diagnosticEmitter.getDiagnosticCount();
1341-
1342-
StackList<BeginBorrowInst *> borrowWorklist(mmci->getFunction());
1343-
1344-
// If we failed to gather borrows due to the transform not understanding
1345-
// part of the SIL, emit a diagnostic, RAUW the mark must check, and
1346-
// continue.
1347-
if (!BorrowToDestructureTransform::gatherBorrows(mmci, borrowWorklist)) {
1348-
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
1349-
mmci->replaceAllUsesWith(mmci->getOperand());
1350-
mmci->eraseFromParent();
1351-
madeChange = true;
1352-
continue;
1353-
}
1354-
1355-
// If we do not have any borrows to process, continue and process the next
1356-
// instruction.
1357-
if (borrowWorklist.empty())
1358-
continue;
1359-
1360-
SmallVector<SILBasicBlock *, 8> discoveredBlocks;
1361-
1362-
// Now that we have found all of our borrows, we want to find struct_extract
1363-
// uses of our borrow as well as any operands that cannot use an owned
1364-
// value.
1365-
SWIFT_DEFER { discoveredBlocks.clear(); };
1366-
BorrowToDestructureTransform transform(allocator, mmci, diagnosticEmitter,
1367-
poa, discoveredBlocks);
1368-
1369-
// Attempt to gather uses. Return if we saw something that we did not
1370-
// understand. Emit a compiler did not understand diagnostic, RAUW the mmci
1371-
// so later passes do not see it, and set madeChange to true.
1372-
if (!transform.gatherUses(borrowWorklist)) {
1373-
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
1374-
mmci->replaceAllUsesWith(mmci->getOperand());
1375-
mmci->eraseFromParent();
1376-
madeChange = true;
1377-
continue;
1378-
}
1379-
1380-
// Next make sure that any destructure needing instructions are on the
1381-
// boundary in a per bit field sensitive manner.
1382-
transform.checkDestructureUsesOnBoundary();
1383-
1384-
// If we emitted any diagnostic, set madeChange to true, eliminate our mmci,
1385-
// and continue.
1386-
if (currentDiagnosticCount != diagnosticEmitter.getDiagnosticCount()) {
1387-
mmci->replaceAllUsesWith(mmci->getOperand());
1388-
mmci->eraseFromParent();
1389-
madeChange = true;
1390-
continue;
1391-
}
1392-
1393-
// At this point, we know that all of our destructure requiring uses are on
1394-
// the boundary of our live range. Now we need to do the rewriting.
1395-
transform.blockToAvailableValues.emplace(transform.liveness);
1396-
transform.rewriteUses();
1397-
1398-
// Now that we have done our rewritting, we need to do a few cleanups.
1399-
//
1400-
// NOTE: We do not eliminate our mark_must_check since we want later passes
1401-
// to do additional checking upon the mark_must_check including making sure
1402-
// that our destructures do not need cause the need for additional copies to
1403-
// be inserted. We only eliminate the mark_must_check if we emitted a
1404-
// diagnostic of some sort.
1405-
transform.cleanup(borrowWorklist);
1406-
madeChange = true;
1407-
}
1408-
1409-
return madeChange;
1410-
}
1411-
1412-
namespace {
1413-
1414-
class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
1415-
void run() override {
1416-
auto *fn = getFunction();
1417-
1418-
// Only run this pass if the move only language feature is enabled.
1419-
if (!fn->getASTContext().LangOpts.Features.contains(Feature::MoveOnly))
1420-
return;
1421-
1422-
// Don't rerun diagnostics on deserialized functions.
1423-
if (getFunction()->wasDeserializedCanonical())
1424-
return;
1425-
1426-
assert(fn->getModule().getStage() == SILStage::Raw &&
1427-
"Should only run on Raw SIL");
1428-
1429-
LLVM_DEBUG(llvm::dbgs()
1430-
<< "===> MoveOnlyBorrowToDestructureTransform. Visiting: "
1431-
<< fn->getName() << '\n');
1432-
1433-
auto *postOrderAnalysis = getAnalysis<PostOrderAnalysis>();
1434-
1435-
SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
1436-
DiagnosticEmitter emitter;
1437-
1438-
bool madeChange = searchForCandidateObjectMarkMustChecks(
1439-
getFunction(), moveIntroducersToProcess, emitter);
1440-
if (madeChange) {
1441-
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
1442-
}
1443-
1444-
if (emitter.emittedAnyDiagnostics()) {
1445-
if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
1446-
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
1447-
return;
1448-
}
1449-
1450-
auto introducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
1451-
moveIntroducersToProcess.end());
1452-
if (runTransform(fn, introducers, postOrderAnalysis, emitter)) {
1453-
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
1454-
}
1455-
1456-
if (emitter.emittedAnyDiagnostics()) {
1457-
if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
1458-
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
1459-
}
1460-
}
1461-
};
1462-
1463-
} // namespace
1464-
1465-
SILTransform *swift::createMoveOnlyBorrowToDestructureTransform() {
1466-
return new MoveOnlyBorrowToDestructureTransformPass();
1467-
}

‎lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureTransformTester.cpp

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
//===--- MoveOnlyBorrowToDestructureTransform.cpp -------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2022 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
///
13+
/// \file This is a pass that converts the borrow + gep pattern to destructures
14+
/// or emits an error if it cannot be done. It is assumed that it runs
15+
/// immediately before move checking of objects runs. This ensures that the move
16+
/// checker does not need to worry about this problem and instead can just check
17+
/// that the newly inserted destructures do not cause move only errors.
18+
///
19+
//===----------------------------------------------------------------------===//
20+
21+
#define DEBUG_TYPE "sil-move-only-checker"
22+
23+
#include "MoveOnlyDiagnostics.h"
24+
#include "MoveOnlyObjectChecker.h"
25+
26+
#include "swift/Basic/Defer.h"
27+
#include "swift/SIL/SILBuilder.h"
28+
#include "swift/SIL/SILInstruction.h"
29+
#include "swift/SILOptimizer/Analysis/Analysis.h"
30+
#include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h"
31+
#include "swift/Basic/BlotSetVector.h"
32+
#include "swift/Basic/FrozenMultiMap.h"
33+
#include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
34+
#include "swift/SILOptimizer/PassManager/Passes.h"
35+
#include "swift/SILOptimizer/PassManager/Transforms.h"
36+
#include "swift/SILOptimizer/Utils/CFGOptUtils.h"
37+
#include "llvm/ADT/ArrayRef.h"
38+
39+
using namespace swift;
40+
using namespace swift::siloptimizer;
41+
42+
//===----------------------------------------------------------------------===//
43+
// Top Level Entrypoint
44+
//===----------------------------------------------------------------------===//
45+
46+
static bool runTransform(SILFunction *fn,
47+
ArrayRef<MarkMustCheckInst *> moveIntroducersToProcess,
48+
PostOrderAnalysis *poa,
49+
DiagnosticEmitter &diagnosticEmitter) {
50+
BorrowToDestructureTransform::IntervalMapAllocator allocator;
51+
bool madeChange = false;
52+
while (!moveIntroducersToProcess.empty()) {
53+
auto *mmci = moveIntroducersToProcess.back();
54+
moveIntroducersToProcess = moveIntroducersToProcess.drop_back();
55+
56+
unsigned currentDiagnosticCount = diagnosticEmitter.getDiagnosticCount();
57+
58+
StackList<BeginBorrowInst *> borrowWorklist(mmci->getFunction());
59+
60+
// If we failed to gather borrows due to the transform not understanding
61+
// part of the SIL, emit a diagnostic, RAUW the mark must check, and
62+
// continue.
63+
if (!BorrowToDestructureTransform::gatherBorrows(mmci, borrowWorklist)) {
64+
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
65+
mmci->replaceAllUsesWith(mmci->getOperand());
66+
mmci->eraseFromParent();
67+
madeChange = true;
68+
continue;
69+
}
70+
71+
// If we do not have any borrows to process, continue and process the next
72+
// instruction.
73+
if (borrowWorklist.empty())
74+
continue;
75+
76+
SmallVector<SILBasicBlock *, 8> discoveredBlocks;
77+
78+
// Now that we have found all of our borrows, we want to find struct_extract
79+
// uses of our borrow as well as any operands that cannot use an owned
80+
// value.
81+
SWIFT_DEFER { discoveredBlocks.clear(); };
82+
BorrowToDestructureTransform transform(allocator, mmci, diagnosticEmitter,
83+
poa, discoveredBlocks);
84+
85+
// Attempt to gather uses. Return if we saw something that we did not
86+
// understand. Emit a compiler did not understand diagnostic, RAUW the mmci
87+
// so later passes do not see it, and set madeChange to true.
88+
if (!transform.gatherUses(borrowWorklist)) {
89+
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(mmci);
90+
mmci->replaceAllUsesWith(mmci->getOperand());
91+
mmci->eraseFromParent();
92+
madeChange = true;
93+
continue;
94+
}
95+
96+
// Next make sure that any destructure needing instructions are on the
97+
// boundary in a per bit field sensitive manner.
98+
transform.checkDestructureUsesOnBoundary();
99+
100+
// If we emitted any diagnostic, set madeChange to true, eliminate our mmci,
101+
// and continue.
102+
if (currentDiagnosticCount != diagnosticEmitter.getDiagnosticCount()) {
103+
mmci->replaceAllUsesWith(mmci->getOperand());
104+
mmci->eraseFromParent();
105+
madeChange = true;
106+
continue;
107+
}
108+
109+
// At this point, we know that all of our destructure requiring uses are on
110+
// the boundary of our live range. Now we need to do the rewriting.
111+
transform.blockToAvailableValues.emplace(transform.liveness);
112+
transform.rewriteUses();
113+
114+
// Now that we have done our rewritting, we need to do a few cleanups.
115+
//
116+
// NOTE: We do not eliminate our mark_must_check since we want later passes
117+
// to do additional checking upon the mark_must_check including making sure
118+
// that our destructures do not need cause the need for additional copies to
119+
// be inserted. We only eliminate the mark_must_check if we emitted a
120+
// diagnostic of some sort.
121+
transform.cleanup(borrowWorklist);
122+
madeChange = true;
123+
}
124+
125+
return madeChange;
126+
}
127+
128+
namespace {
129+
130+
class MoveOnlyBorrowToDestructureTransformPass : public SILFunctionTransform {
131+
void run() override {
132+
auto *fn = getFunction();
133+
134+
// Only run this pass if the move only language feature is enabled.
135+
if (!fn->getASTContext().LangOpts.Features.contains(Feature::MoveOnly))
136+
return;
137+
138+
// Don't rerun diagnostics on deserialized functions.
139+
if (getFunction()->wasDeserializedCanonical())
140+
return;
141+
142+
assert(fn->getModule().getStage() == SILStage::Raw &&
143+
"Should only run on Raw SIL");
144+
145+
LLVM_DEBUG(llvm::dbgs()
146+
<< "===> MoveOnlyBorrowToDestructureTransform. Visiting: "
147+
<< fn->getName() << '\n');
148+
149+
auto *postOrderAnalysis = getAnalysis<PostOrderAnalysis>();
150+
151+
SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
152+
DiagnosticEmitter emitter;
153+
154+
bool madeChange = searchForCandidateObjectMarkMustChecks(
155+
getFunction(), moveIntroducersToProcess, emitter);
156+
if (madeChange) {
157+
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
158+
}
159+
160+
if (emitter.emittedAnyDiagnostics()) {
161+
if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
162+
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
163+
return;
164+
}
165+
166+
auto introducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
167+
moveIntroducersToProcess.end());
168+
if (runTransform(fn, introducers, postOrderAnalysis, emitter)) {
169+
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
170+
}
171+
172+
if (emitter.emittedAnyDiagnostics()) {
173+
if (cleanupSILAfterEmittingObjectMoveOnlyDiagnostics(fn))
174+
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
175+
}
176+
}
177+
};
178+
179+
} // namespace
180+
181+
SILTransform *swift::createMoveOnlyBorrowToDestructureTransform() {
182+
return new MoveOnlyBorrowToDestructureTransformPass();
183+
}

0 commit comments

Comments
 (0)