Skip to content

Commit eb1f02f

Browse files
committed
[move-only] Improve logging to make it more readable and add a counter to bisect on variables that we are checking.
These are only on in NDEBUG mode. It just makes it easier to quickly triage which variable is causing a checking problem. (cherry picked from commit fb487bf)
1 parent b624759 commit eb1f02f

File tree

1 file changed

+86
-18
lines changed

1 file changed

+86
-18
lines changed

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

Lines changed: 86 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,22 @@ llvm::cl::opt<bool> DisableMoveOnlyAddressCheckerLifetimeExtension(
290290
// MARK: Utilities
291291
//===----------------------------------------------------------------------===//
292292

293+
struct RAIILLVMDebug {
294+
StringRef str;
295+
296+
RAIILLVMDebug(StringRef str) : str(str) {
297+
LLVM_DEBUG(llvm::dbgs() << "===>>> Starting " << str << '\n');
298+
}
299+
300+
RAIILLVMDebug(StringRef str, SILInstruction *u) : str(str) {
301+
LLVM_DEBUG(llvm::dbgs() << "===>>> Starting " << str << ":" << *u);
302+
}
303+
304+
~RAIILLVMDebug() {
305+
LLVM_DEBUG(llvm::dbgs() << "===<<< Completed " << str << '\n');
306+
}
307+
};
308+
293309
static void insertDebugValueBefore(SILInstruction *insertPt,
294310
DebugVarCarryingInst debugVar,
295311
llvm::function_ref<SILValue ()> operand) {
@@ -3121,6 +3137,8 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
31213137
// [copy] + begin_borrow for further processing. This just eliminates a case
31223138
// that the checker doesn't need to know about.
31233139
{
3140+
RAIILLVMDebug l("CopiedLoadBorrowEliminationVisitor");
3141+
31243142
CopiedLoadBorrowEliminationState state(markedAddress->getFunction());
31253143
CopiedLoadBorrowEliminationVisitor copiedLoadBorrowEliminator(state);
31263144
if (AddressUseKind::Unknown ==
@@ -3142,12 +3160,16 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
31423160
// SILGen will treat y as a separate rvalue from x and will create a temporary
31433161
// allocation. In contrast if we have a var, we treat x like an lvalue and
31443162
// just create GEPs appropriately.
3145-
if (eliminateTemporaryAllocationsFromLet(markedAddress)) {
3146-
LLVM_DEBUG(
3147-
llvm::dbgs()
3148-
<< "Succeeded in eliminating temporary allocations! Fn after:\n";
3149-
markedAddress->getFunction()->dump());
3150-
changed = true;
3163+
{
3164+
RAIILLVMDebug l("temporary allocations from rvalue accesses");
3165+
3166+
if (eliminateTemporaryAllocationsFromLet(markedAddress)) {
3167+
LLVM_DEBUG(
3168+
llvm::dbgs()
3169+
<< "Succeeded in eliminating temporary allocations! Fn after:\n";
3170+
markedAddress->getFunction()->dump());
3171+
changed = true;
3172+
}
31513173
}
31523174

31533175
// Then gather all uses of our address by walking from def->uses. We use this
@@ -3156,10 +3178,16 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
31563178
GatherUsesVisitor visitor(*this, addressUseState, markedAddress,
31573179
diagnosticEmitter);
31583180
SWIFT_DEFER { visitor.clear(); };
3159-
visitor.reset(markedAddress);
3160-
if (AddressUseKind::Unknown == std::move(visitor).walk(markedAddress)) {
3161-
LLVM_DEBUG(llvm::dbgs() << "Failed access path visit: " << *markedAddress);
3162-
return false;
3181+
3182+
{
3183+
RAIILLVMDebug l("main use gathering visitor");
3184+
3185+
visitor.reset(markedAddress);
3186+
if (AddressUseKind::Unknown == std::move(visitor).walk(markedAddress)) {
3187+
LLVM_DEBUG(llvm::dbgs()
3188+
<< "Failed access path visit: " << *markedAddress);
3189+
return false;
3190+
}
31633191
}
31643192

31653193
// If we found a load [copy] or copy_addr that requires multiple copies or an
@@ -3187,18 +3215,28 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
31873215
//===---
31883216
// Liveness Checking
31893217
//
3218+
31903219
SmallVector<SILBasicBlock *, 32> discoveredBlocks;
31913220
FieldSensitiveMultiDefPrunedLiveRange liveness(fn, markedAddress,
31923221
&discoveredBlocks);
3193-
addressUseState.initializeLiveness(liveness);
31943222

3195-
// Then compute the takes that are within the cumulative boundary of
3196-
// liveness that we have computed. If we find any, they are the errors ones.
3197-
GlobalLivenessChecker emitter(addressUseState, diagnosticEmitter, liveness);
3223+
{
3224+
RAIILLVMDebug logger("liveness initialization");
31983225

3199-
// If we had any errors, we do not want to modify the SIL... just bail.
3200-
if (emitter.compute()) {
3201-
return true;
3226+
addressUseState.initializeLiveness(liveness);
3227+
}
3228+
3229+
{
3230+
RAIILLVMDebug l("global liveness checking");
3231+
3232+
// Then compute the takes that are within the cumulative boundary of
3233+
// liveness that we have computed. If we find any, they are the errors ones.
3234+
GlobalLivenessChecker emitter(addressUseState, diagnosticEmitter, liveness);
3235+
3236+
// If we had any errors, we do not want to modify the SIL... just bail.
3237+
if (emitter.compute()) {
3238+
return true;
3239+
}
32023240
}
32033241

32043242
//===
@@ -3242,15 +3280,40 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
32423280
// MARK: Top Level Entrypoint
32433281
//===----------------------------------------------------------------------===//
32443282

3283+
#ifndef NDEBUG
3284+
static llvm::cl::opt<uint64_t> NumTopLevelToProcess(
3285+
"sil-move-only-address-checker-num-top-level-to-process",
3286+
llvm::cl::desc("Allows for bisecting on move introducer that causes an "
3287+
"error. Only meant for debugging!"),
3288+
llvm::cl::init(UINT64_MAX));
3289+
#endif
3290+
3291+
static llvm::cl::opt<bool> DumpSILBeforeRemovingMarkMustCheck(
3292+
"sil-move-only-address-checker-dump-before-removing-mark-must-check",
3293+
llvm::cl::desc("When bisecting it is useful to dump the SIL before the "
3294+
"rest of the checker removes mark_must_check. This lets one "
3295+
"grab the SIL of a bad variable after all of the rest have "
3296+
"been processed to work with further in sil-opt."),
3297+
llvm::cl::init(false));
3298+
32453299
bool MoveOnlyAddressChecker::check(
32463300
SmallSetVector<MarkMustCheckInst *, 32> &moveIntroducersToProcess) {
32473301
assert(moveIntroducersToProcess.size() &&
32483302
"Must have checks to process to call this function");
32493303
MoveOnlyAddressCheckerPImpl pimpl(fn, diagnosticEmitter, domTree, poa,
32503304
allocator);
32513305

3306+
#ifndef NDEBUG
3307+
static uint64_t numProcessed = 0;
3308+
#endif
32523309
for (auto *markedValue : moveIntroducersToProcess) {
3253-
LLVM_DEBUG(llvm::dbgs() << "Visiting: " << *markedValue);
3310+
#ifndef NDEBUG
3311+
++numProcessed;
3312+
if (NumTopLevelToProcess <= numProcessed)
3313+
break;
3314+
#endif
3315+
LLVM_DEBUG(llvm::dbgs()
3316+
<< "======>>> Visiting top level: " << *markedValue);
32543317

32553318
// Perform our address check.
32563319
unsigned diagnosticEmittedByEarlierPassCount =
@@ -3271,5 +3334,10 @@ bool MoveOnlyAddressChecker::check(
32713334
}
32723335
}
32733336

3337+
if (DumpSILBeforeRemovingMarkMustCheck) {
3338+
LLVM_DEBUG(llvm::dbgs()
3339+
<< "Dumping SIL before removing mark must checks!\n";
3340+
fn->dump());
3341+
}
32743342
return pimpl.changed;
32753343
}

0 commit comments

Comments
 (0)