Skip to content

Commit 2a8f069

Browse files
gottesmmgottesmm
committed
[move-only-object] Refactor diagnostic emission so I can add more logic to improve QoI.
Specifically, I want to add a special diagnostic for closure captures. By doing this as a separate commit it is easier to review/etc.
1 parent 0b05a1e commit 2a8f069

File tree

1 file changed

+179
-105
lines changed

1 file changed

+179
-105
lines changed

‎lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.cpp

Lines changed: 179 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,70 @@
4141
using namespace swift;
4242

4343
//===----------------------------------------------------------------------===//
44-
// Utilities
44+
// MARK: Checker State
45+
//===----------------------------------------------------------------------===//
46+
47+
namespace {
48+
49+
/// Wrapper around CanonicalizeOSSALifetime that we use to specialize its
50+
/// interface for our purposes.
51+
struct OSSACanonicalizer {
52+
/// A per mark must check, vector of uses that copy propagation says need a
53+
/// copy and thus are not final consuming uses.
54+
SmallVector<Operand *, 32> consumingUsesNeedingCopy;
55+
56+
/// A per mark must check, vector of consuming uses that copy propagation says
57+
/// are actual last uses.
58+
SmallVector<Operand *, 32> finalConsumingUses;
59+
60+
/// The actual canonicalizer that we use.
61+
///
62+
/// We mark this Optional to avoid UB behavior caused by us needing to
63+
/// initialize CanonicalizeOSSALifetime with parts of OSSACanoncializer
64+
/// (specifically with state in our arrays) before the actual constructor has
65+
/// run. Specifically this avoids:
66+
///
67+
/// 11.9.5p1 class.cdtor: For an object with a non-trivial constructor,
68+
/// referring to any non-static member or base class of the object before the
69+
/// constructor begins execution results in undefined behavior.
70+
Optional<CanonicalizeOSSALifetime> canonicalizer;
71+
72+
OSSACanonicalizer(SILFunction *fn,
73+
NonLocalAccessBlockAnalysis *accessBlockAnalysis,
74+
DominanceInfo *domTree, InstructionDeleter &deleter) {
75+
auto foundConsumingUseNeedingCopy = std::function<void(Operand *)>(
76+
[&](Operand *use) { consumingUsesNeedingCopy.push_back(use); });
77+
auto foundConsumingUseNotNeedingCopy = std::function<void(Operand *)>(
78+
[&](Operand *use) { finalConsumingUses.push_back(use); });
79+
80+
canonicalizer.emplace(
81+
false /*pruneDebugMode*/, !fn->shouldOptimize() /*maximizeLifetime*/,
82+
accessBlockAnalysis, domTree, deleter, foundConsumingUseNeedingCopy,
83+
foundConsumingUseNotNeedingCopy);
84+
}
85+
86+
void clear() {
87+
consumingUsesNeedingCopy.clear();
88+
finalConsumingUses.clear();
89+
}
90+
91+
bool canonicalize(MarkMustCheckInst *markedValue) {
92+
return canonicalizer->canonicalizeValueLifetime(markedValue);
93+
}
94+
95+
bool foundAnyConsumingUses() const {
96+
return consumingUsesNeedingCopy.size() || finalConsumingUses.size();
97+
}
98+
99+
bool foundConsumingUseRequiringCopy() const {
100+
return consumingUsesNeedingCopy.size();
101+
}
102+
};
103+
104+
} // anonymous namespace
105+
106+
//===----------------------------------------------------------------------===//
107+
// MARK: Diagnostic Utilities
45108
//===----------------------------------------------------------------------===//
46109

47110
template <typename... T, typename... U>
@@ -66,8 +129,110 @@ static StringRef getVariableNameForValue(MarkMustCheckInst *mmci) {
66129
return varName;
67130
}
68131

132+
namespace {
133+
134+
struct DiagnosticEmitter {
135+
SILFunction *fn;
136+
const OSSACanonicalizer &canonicalizer;
137+
SmallPtrSet<MarkMustCheckInst *, 4> valuesWithDiagnostics;
138+
139+
void emitCheckerDoesntUnderstandDiagnostic(MarkMustCheckInst *markedValue);
140+
void emitGuaranteedDiagnostic(MarkMustCheckInst *markedValue);
141+
void emitOwnedDiagnostic(MarkMustCheckInst *markedValue);
142+
143+
bool emittedAnyDiagnostics() const { return valuesWithDiagnostics.size(); }
144+
145+
bool emittedDiagnosticForValue(MarkMustCheckInst *markedValue) const {
146+
return valuesWithDiagnostics.count(markedValue);
147+
}
148+
149+
private:
150+
void emitDiagnosticsForFoundUses() const;
151+
};
152+
153+
} // anonymous namespace
154+
155+
void DiagnosticEmitter::emitCheckerDoesntUnderstandDiagnostic(
156+
MarkMustCheckInst *markedValue) {
157+
// If we failed to canonicalize ownership, there was something in the SIL
158+
// that copy propagation did not understand. Emit a we did not understand
159+
// error.
160+
if (markedValue->getType().isMoveOnlyWrapped()) {
161+
diagnose(fn->getASTContext(), markedValue->getLoc().getSourceLoc(),
162+
diag::sil_moveonlychecker_not_understand_no_implicit_copy);
163+
} else {
164+
diagnose(fn->getASTContext(), markedValue->getLoc().getSourceLoc(),
165+
diag::sil_moveonlychecker_not_understand_moveonly);
166+
}
167+
valuesWithDiagnostics.insert(markedValue);
168+
}
169+
170+
void DiagnosticEmitter::emitGuaranteedDiagnostic(
171+
MarkMustCheckInst *markedValue) {
172+
auto &astContext = fn->getASTContext();
173+
StringRef varName = getVariableNameForValue(markedValue);
174+
175+
diagnose(astContext,
176+
markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
177+
diag::sil_moveonlychecker_guaranteed_value_consumed, varName);
178+
179+
emitDiagnosticsForFoundUses();
180+
valuesWithDiagnostics.insert(markedValue);
181+
}
182+
183+
void DiagnosticEmitter::emitOwnedDiagnostic(MarkMustCheckInst *markedValue) {
184+
auto &astContext = fn->getASTContext();
185+
StringRef varName = getVariableNameForValue(markedValue);
186+
187+
diagnose(astContext,
188+
markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
189+
diag::sil_moveonlychecker_owned_value_consumed_more_than_once,
190+
varName);
191+
192+
emitDiagnosticsForFoundUses();
193+
valuesWithDiagnostics.insert(markedValue);
194+
}
195+
196+
void DiagnosticEmitter::emitDiagnosticsForFoundUses() const {
197+
auto &astContext = fn->getASTContext();
198+
199+
for (auto *consumingUse : canonicalizer.consumingUsesNeedingCopy) {
200+
// See if the consuming use is an owned moveonly_to_copyable whose only
201+
// user is a return. In that case, use the return loc instead. We do this
202+
// b/c it is illegal to put a return value location on a non-return value
203+
// instruction... so we have to hack around this slightly.
204+
auto *user = consumingUse->getUser();
205+
auto loc = user->getLoc();
206+
if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
207+
if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
208+
loc = ri->getLoc();
209+
}
210+
}
211+
212+
diagnose(astContext, loc.getSourceLoc(),
213+
diag::sil_moveonlychecker_consuming_use_here);
214+
}
215+
216+
for (auto *consumingUse : canonicalizer.finalConsumingUses) {
217+
// See if the consuming use is an owned moveonly_to_copyable whose only
218+
// user is a return. In that case, use the return loc instead. We do this
219+
// b/c it is illegal to put a return value location on a non-return value
220+
// instruction... so we have to hack around this slightly.
221+
auto *user = consumingUse->getUser();
222+
auto loc = user->getLoc();
223+
if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
224+
if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
225+
loc = ri->getLoc();
226+
}
227+
}
228+
229+
diagnose(astContext, loc.getSourceLoc(),
230+
diag::sil_moveonlychecker_consuming_use_here);
231+
}
232+
}
233+
69234
//===----------------------------------------------------------------------===//
70-
// Main Pass
235+
// MARK: Main Pass
71236
//===----------------------------------------------------------------------===//
72237

73238
namespace {
@@ -78,14 +243,6 @@ struct MoveOnlyChecker {
78243
/// A set of mark_must_check that we are actually going to process.
79244
SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
80245

81-
/// A per mark must check, vector of uses that copy propagation says need a
82-
/// copy and thus are not final consuming uses.
83-
SmallVector<Operand *, 32> consumingUsesNeedingCopy;
84-
85-
/// A per mark must check, vector of consuming uses that copy propagation says
86-
/// are actual last uses.
87-
SmallVector<Operand *, 32> finalConsumingUses;
88-
89246
MoveOnlyChecker(SILFunction *fn, DeadEndBlocks *deBlocks) : fn(fn) {}
90247

91248
/// Search through the current function for candidate mark_must_check
@@ -97,10 +254,6 @@ struct MoveOnlyChecker {
97254
/// recognize after emitting the diagnostic.
98255
bool searchForCandidateMarkMustChecks();
99256

100-
/// Emits an error diagnostic for \p markedValue.
101-
void emitDiagnostic(MarkMustCheckInst *markedValue,
102-
bool originalValueGuaranteed);
103-
104257
bool check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
105258
DominanceInfo *domTree);
106259
};
@@ -314,61 +467,6 @@ bool MoveOnlyChecker::searchForCandidateMarkMustChecks() {
314467
return changed;
315468
}
316469

317-
void MoveOnlyChecker::emitDiagnostic(MarkMustCheckInst *markedValue,
318-
bool originalValueGuaranteed) {
319-
auto &astContext = fn->getASTContext();
320-
StringRef varName = getVariableNameForValue(markedValue);
321-
322-
if (originalValueGuaranteed) {
323-
diagnose(astContext,
324-
markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
325-
diag::sil_moveonlychecker_guaranteed_value_consumed, varName);
326-
} else {
327-
diagnose(astContext,
328-
markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
329-
diag::sil_moveonlychecker_owned_value_consumed_more_than_once,
330-
varName);
331-
}
332-
333-
while (consumingUsesNeedingCopy.size()) {
334-
auto *consumingUse = consumingUsesNeedingCopy.pop_back_val();
335-
336-
// See if the consuming use is an owned moveonly_to_copyable whose only
337-
// user is a return. In that case, use the return loc instead. We do this
338-
// b/c it is illegal to put a return value location on a non-return value
339-
// instruction... so we have to hack around this slightly.
340-
auto *user = consumingUse->getUser();
341-
auto loc = user->getLoc();
342-
if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
343-
if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
344-
loc = ri->getLoc();
345-
}
346-
}
347-
348-
diagnose(astContext, loc.getSourceLoc(),
349-
diag::sil_moveonlychecker_consuming_use_here);
350-
}
351-
352-
while (finalConsumingUses.size()) {
353-
auto *consumingUse = finalConsumingUses.pop_back_val();
354-
355-
// See if the consuming use is an owned moveonly_to_copyable whose only
356-
// user is a return. In that case, use the return loc instead. We do this
357-
// b/c it is illegal to put a return value location on a non-return value
358-
// instruction... so we have to hack around this slightly.
359-
auto *user = consumingUse->getUser();
360-
auto loc = user->getLoc();
361-
if (auto *mtc = dyn_cast<MoveOnlyWrapperToCopyableValueInst>(user)) {
362-
if (auto *ri = mtc->getSingleUserOfType<ReturnInst>()) {
363-
loc = ri->getLoc();
364-
}
365-
}
366-
367-
diagnose(astContext, loc.getSourceLoc(),
368-
diag::sil_moveonlychecker_consuming_use_here);
369-
}
370-
}
371-
372470
bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
373471
DominanceInfo *domTree) {
374472
bool changed = false;
@@ -392,44 +490,21 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
392490
instToDelete->eraseFromParent();
393491
});
394492
InstructionDeleter deleter(std::move(callbacks));
493+
OSSACanonicalizer canonicalizer(fn, accessBlockAnalysis, domTree, deleter);
494+
DiagnosticEmitter diagnosticEmitter{fn, canonicalizer, {}};
395495

396-
auto foundConsumingUseNeedingCopy = [&](Operand *use) {
397-
consumingUsesNeedingCopy.push_back(use);
398-
};
399-
auto foundConsumingUseNotNeedingCopy = [&](Operand *use) {
400-
finalConsumingUses.push_back(use);
401-
};
402-
403-
CanonicalizeOSSALifetime canonicalizer(
404-
false /*pruneDebugMode*/, !fn->shouldOptimize() /*maximizeLifetime*/,
405-
accessBlockAnalysis, domTree, deleter, foundConsumingUseNeedingCopy,
406-
foundConsumingUseNotNeedingCopy);
407496
auto moveIntroducers = llvm::makeArrayRef(moveIntroducersToProcess.begin(),
408497
moveIntroducersToProcess.end());
409-
SmallPtrSet<MarkMustCheckInst *, 4> valuesWithDiagnostics;
410498
while (!moveIntroducers.empty()) {
411-
SWIFT_DEFER {
412-
consumingUsesNeedingCopy.clear();
413-
finalConsumingUses.clear();
414-
};
499+
SWIFT_DEFER { canonicalizer.clear(); };
415500

416501
MarkMustCheckInst *markedValue = moveIntroducers.front();
417502
moveIntroducers = moveIntroducers.drop_front(1);
418503
LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *markedValue);
419504

420505
// First canonicalize ownership.
421-
if (!canonicalizer.canonicalizeValueLifetime(markedValue)) {
422-
// If we failed to canonicalize ownership, there was something in the SIL
423-
// that copy propagation did not understand. Emit a we did not understand
424-
// error.
425-
if (markedValue->getType().isMoveOnlyWrapped()) {
426-
diagnose(fn->getASTContext(), markedValue->getLoc().getSourceLoc(),
427-
diag::sil_moveonlychecker_not_understand_no_implicit_copy);
428-
} else {
429-
diagnose(fn->getASTContext(), markedValue->getLoc().getSourceLoc(),
430-
diag::sil_moveonlychecker_not_understand_moveonly);
431-
}
432-
valuesWithDiagnostics.insert(markedValue);
506+
if (!canonicalizer.canonicalize(markedValue)) {
507+
diagnosticEmitter.emitCheckerDoesntUnderstandDiagnostic(markedValue);
433508
continue;
434509
} else {
435510
// Always set changed to true if we succeeded in canonicalizing since we
@@ -440,25 +515,24 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
440515
// If we are asked to perform guaranteed checking, emit an error if we have
441516
// /any/ consuming uses.
442517
if (markedValue->getCheckKind() == MarkMustCheckInst::CheckKind::NoCopy) {
443-
if (!consumingUsesNeedingCopy.empty() || !finalConsumingUses.empty()) {
444-
emitDiagnostic(markedValue, true /*original value guaranteed*/);
445-
valuesWithDiagnostics.insert(markedValue);
518+
if (canonicalizer.foundAnyConsumingUses()) {
519+
diagnosticEmitter.emitGuaranteedDiagnostic(markedValue);
446520
}
447521
continue;
448522
}
449523

450-
if (consumingUsesNeedingCopy.empty()) {
524+
if (!canonicalizer.foundConsumingUseRequiringCopy()) {
451525
// If we failed to understand how to perform the check or did not find
452526
// any targets... continue. In the former case we want to fail with a
453527
// checker did not understand diagnostic later and in the former, we
454528
// succeeded.
455529
continue;
456530
}
457531

458-
emitDiagnostic(markedValue, false /*original value guaranteed*/);
459-
valuesWithDiagnostics.insert(markedValue);
532+
diagnosticEmitter.emitOwnedDiagnostic(markedValue);
460533
}
461-
bool emittedDiagnostic = valuesWithDiagnostics.size();
534+
535+
bool emittedDiagnostic = diagnosticEmitter.emittedAnyDiagnostics();
462536

463537
// Ok, we have success. All of our marker instructions were proven as safe or
464538
// we emitted a diagnostic. Now we need to clean up the IR by eliminating our
@@ -477,7 +551,7 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
477551

478552
// If we didn't emit a diagnostic on a non-trivial guaranteed argument,
479553
// eliminate the copy_value, destroy_values, and the mark_must_check.
480-
if (!valuesWithDiagnostics.count(markedInst)) {
554+
if (!diagnosticEmitter.emittedDiagnosticForValue(markedInst)) {
481555
if (markedInst->getCheckKind() == MarkMustCheckInst::CheckKind::NoCopy) {
482556
if (auto *cvi = dyn_cast<CopyValueInst>(markedInst->getOperand())) {
483557
if (auto *arg = dyn_cast<SILFunctionArgument>(cvi->getOperand())) {

0 commit comments

Comments
 (0)