Skip to content

[DebugInfo][RemoveDIs] Make dropping variable locations explicit #72399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 21, 2023
Merged

[DebugInfo][RemoveDIs] Make dropping variable locations explicit #72399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/lib/IR/DebugInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ bool llvm::stripDebugInfo(Function &F) {
// DIAssignID are debug info metadata primitives.
I.setMetadata(LLVMContext::MD_DIAssignID, nullptr);
}
I.dropDbgValues();
}
}
return Changed;
Expand Down
10 changes: 10 additions & 0 deletions llvm/lib/Transforms/Scalar/ADCE.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,16 @@ ADCEChanged AggressiveDeadCodeElimination::removeDeadInstructions() {
// value of the function, and may therefore be deleted safely.
// NOTE: We reuse the Worklist vector here for memory efficiency.
for (Instruction &I : llvm::reverse(instructions(F))) {
// With "RemoveDIs" debug-info stored in DPValue objects, debug-info
// attached to this instruction, and drop any for scopes that aren't alive,
// like the rest of this loop does. Extending support to assignment tracking
// is future work.
for (DPValue &DPV : make_early_inc_range(I.getDbgValueRange())) {
if (AliveScopes.count(DPV.getDebugLoc()->getScope()))
continue;
I.dropOneDbgValue(&DPV);
}

// Check if the instruction is alive.
if (isLive(&I))
continue;
Expand Down
18 changes: 18 additions & 0 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3122,6 +3122,11 @@ bool SimplifyCFGOpt::SpeculativelyExecuteBB(BranchInst *BI,
}

// Hoist the instructions.
// In "RemoveDIs" non-instr debug-info mode, drop DPValues attached to these
// instructions, in the same way that dbg.value intrinsics are dropped at the
// end of this block.
for (auto &It : make_range(ThenBB->begin(), ThenBB->end()))
It.dropDbgValues();
BB->splice(BI->getIterator(), ThenBB, ThenBB->begin(),
std::prev(ThenBB->end()));

Expand Down Expand Up @@ -5179,6 +5184,15 @@ bool SimplifyCFGOpt::simplifyUnreachable(UnreachableInst *UI) {

bool Changed = false;

// Ensure that any debug-info records that used to occur after the Unreachable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few lines up there's an ominous comment // WARNING: keep in sync with InstCombinerImpl::visitUnreachableInst()! - does anything over there need changing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, I believe it's covered by #72380 , not quite sure why I split these patches to be separate.

// are moved to in front of it -- otherwise they'll "dangle" at the end of
// the block.
BB->flushTerminatorDbgValues();

// Debug-info records on the unreachable inst itself should be deleted, as
// below we delete everything past the final executable instruction.
UI->dropDbgValues();
Comment on lines +5192 to +5194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't the DPValues on the unreachable come "before" it though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do, and it's arguable that we should keep them, however that's not what the current behaviour with dbg.values is -- the loop immediately below these lines iterates through and deletes any instructions that don't call/branch/signal etc, i.e. anything where it's inevitable that the unreachable will be hit. That includes dbg.values, which get deleted, which is what this addition preserves.


// If there are any instructions immediately before the unreachable that can
// be removed, do so.
while (UI->getIterator() != BB->begin()) {
Expand All @@ -5195,6 +5209,10 @@ bool SimplifyCFGOpt::simplifyUnreachable(UnreachableInst *UI) {
// block will be the unwind edges of Invoke/CatchSwitch/CleanupReturn,
// and we can therefore guarantee this block will be erased.

// If we're deleting this, we're deleting any subsequent dbg.values, so
// delete DPValue records of variable information.
BBI->dropDbgValues();

// Delete this instruction (any uses are guaranteed to be dead)
BBI->replaceAllUsesWith(PoisonValue::get(BBI->getType()));
BBI->eraseFromParent();
Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/ADCE/adce-salvage-dbg-value.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
;; Check that adce salvages debug info properly.
; RUN: opt -passes=adce -S < %s | FileCheck %s
; RUN: opt -passes=adce -S < %s --try-experimental-debuginfo-iterators| FileCheck %s

; ModuleID = 'test.ll'
source_filename = "test.ll"
Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/ADCE/debug-info-intrinsic.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
; RUN: opt -passes=adce -S < %s | FileCheck %s
; RUN: opt -passes=adce -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
; Test that debug info intrinsics in dead scopes get eliminated by -adce.

; Generated with 'clang -g -S -emit-llvm | opt -passes=mem2reg -inline' at r262899
Expand Down
3 changes: 3 additions & 0 deletions llvm/test/Transforms/SimplifyCFG/return-merge.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
; RUN: opt < %s -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck --check-prefixes=CHECK %s
; RUN: opt < %s -passes=debugify,simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck --check-prefixes=DBGINFO %s

; RUN: opt < %s -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S --try-experimental-debuginfo-iterators | FileCheck --check-prefixes=CHECK %s
; RUN: opt < %s -passes=debugify,simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S --try-experimental-debuginfo-iterators | FileCheck --check-prefixes=DBGINFO %s

define i32 @test1(i1 %C) {
; CHECK-LABEL: @test1(
; CHECK-NEXT: entry:
Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/SimplifyCFG/speculate-dbgvalue.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 | FileCheck %s
; RUN: opt < %s -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 --try-experimental-debuginfo-iterators | FileCheck %s

; This test case was generated from speculate-dbgvalue.c:
;
Expand Down