[DebugInfo][RemoveDIs] Make dropping variable locations explicit #72399
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In present-day debug-info, when you delete all instructions, you delete all their debug-info with it because debug-info is stored in instructions. With debug-info stored in DPValue objects however, deleting instructions causes DPValue objects to clump together into a large blob of debug-info that hangs around in the block, as nothing has explicitly deleted it. To restore this behaviour, scatter calls to dropDbgValues around in places that used to delete chunks of dbg.values, for example during stripDebugInfo and in the code that deletes everything after an Unreachable instruction. DCE is another example. The tests with --try... added to them are new scenariso where we can now correctly replicate the "normal" debug-info behaviour. Alas, there's no explicit test for the opt -strip-debug option though (in dbg.value mode or DPValue mode).
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesIn present-day debug-info, when you delete all instructions, you delete all their debug-info with it because debug-info is stored in instructions. With debug-info stored in DPValue objects however, deleting instructions causes DPValue objects to clump together into a large blob of debug-info that hangs around in the block, as nothing has explicitly deleted it. To restore this behaviour, scatter calls to dropDbgValues around in places that used to delete chunks of dbg.values, for example during stripDebugInfo and in the code that deletes everything after an Unreachable instruction. DCE is another example. The tests with --try... added to them are new scenariso where we can now correctly replicate the "normal" debug-info behaviour. Alas, there's no explicit test for the opt -strip-debug option though (in dbg.value mode or DPValue mode). Full diff: https://github.com/llvm/llvm-project/pull/72399.diff 7 Files Affected:
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 390a27c4bc0c4dd..2f35abd5bae0a5d 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -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;
diff --git a/llvm/lib/Transforms/Scalar/ADCE.cpp b/llvm/lib/Transforms/Scalar/ADCE.cpp
index 24354211341f822..9af275a9f4e204a 100644
--- a/llvm/lib/Transforms/Scalar/ADCE.cpp
+++ b/llvm/lib/Transforms/Scalar/ADCE.cpp
@@ -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;
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 4dae52a8ecffdf6..df2fd700a105e71 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -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()));
@@ -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
+ // 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();
+
// If there are any instructions immediately before the unreachable that can
// be removed, do so.
while (UI->getIterator() != BB->begin()) {
@@ -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();
diff --git a/llvm/test/Transforms/ADCE/adce-salvage-dbg-value.ll b/llvm/test/Transforms/ADCE/adce-salvage-dbg-value.ll
index ac2fac9832aa616..8af6f01e4f26fc1 100644
--- a/llvm/test/Transforms/ADCE/adce-salvage-dbg-value.ll
+++ b/llvm/test/Transforms/ADCE/adce-salvage-dbg-value.ll
@@ -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"
diff --git a/llvm/test/Transforms/ADCE/debug-info-intrinsic.ll b/llvm/test/Transforms/ADCE/debug-info-intrinsic.ll
index 34c09e2c6f6d36e..5d87a1cbbe0cb44 100644
--- a/llvm/test/Transforms/ADCE/debug-info-intrinsic.ll
+++ b/llvm/test/Transforms/ADCE/debug-info-intrinsic.ll
@@ -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
diff --git a/llvm/test/Transforms/SimplifyCFG/return-merge.ll b/llvm/test/Transforms/SimplifyCFG/return-merge.ll
index cd8e88eb6e1e6a3..287a112008d0c9a 100644
--- a/llvm/test/Transforms/SimplifyCFG/return-merge.ll
+++ b/llvm/test/Transforms/SimplifyCFG/return-merge.ll
@@ -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:
diff --git a/llvm/test/Transforms/SimplifyCFG/speculate-dbgvalue.ll b/llvm/test/Transforms/SimplifyCFG/speculate-dbgvalue.ll
index eb62d6c613c9091..ba26c70962d7303 100644
--- a/llvm/test/Transforms/SimplifyCFG/speculate-dbgvalue.ll
+++ b/llvm/test/Transforms/SimplifyCFG/speculate-dbgvalue.ll
@@ -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:
;
|
You can test this locally with the following command:git-clang-format --diff ac378ac493426f8094cfaa176f1e88b62914f630 b79a83434c49f717901f0ad679ddb4e90c501411 -- llvm/lib/IR/DebugInfo.cpp llvm/lib/Transforms/Scalar/ADCE.cpp llvm/lib/Transforms/Utils/SimplifyCFG.cppView the diff from clang-format here.diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index 2f35abd5ba..9114f69412 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1316,17 +1316,15 @@ LLVMDIBuilderCreateUnspecifiedType(LLVMDIBuilderRef Builder, const char *Name,
return wrap(unwrap(Builder)->createUnspecifiedType({Name, NameLen}));
}
-LLVMMetadataRef
-LLVMDIBuilderCreateStaticMemberType(
+LLVMMetadataRef LLVMDIBuilderCreateStaticMemberType(
LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
LLVMMetadataRef Type, LLVMDIFlags Flags, LLVMValueRef ConstantVal,
uint32_t AlignInBits) {
return wrap(unwrap(Builder)->createStaticMemberType(
- unwrapDI<DIScope>(Scope), {Name, NameLen},
- unwrapDI<DIFile>(File), LineNumber, unwrapDI<DIType>(Type),
- map_from_llvmDIFlags(Flags), unwrap<Constant>(ConstantVal),
- AlignInBits));
+ unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File),
+ LineNumber, unwrapDI<DIType>(Type), map_from_llvmDIFlags(Flags),
+ unwrap<Constant>(ConstantVal), AlignInBits));
}
LLVMMetadataRef
|
OCHyams
approved these changes
Nov 15, 2023
| @@ -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 | |||
There was a problem hiding this comment.
A few lines up there's an ominous comment // WARNING: keep in sync with InstCombinerImpl::visitUnreachableInst()! - does anything over there need changing?
There was a problem hiding this comment.
It does, I believe it's covered by #72380 , not quite sure why I split these patches to be separate.
Comment on lines
+5192
to
+5194
| // Debug-info records on the unreachable inst itself should be deleted, as | ||
| // below we delete everything past the final executable instruction. | ||
| UI->dropDbgValues(); |
There was a problem hiding this comment.
Don't the DPValues on the unreachable come "before" it though?
There was a problem hiding this comment.
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.
|
(Ignoring the formatting errors seeing how it's in code I'm not touching) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In present-day debug-info, when you delete all instructions, you delete all their debug-info with it because debug-info is stored in instructions. With debug-info stored in DPValue objects however, deleting instructions causes DPValue objects to clump together into a large blob of debug-info that hangs around in the block, as nothing has explicitly deleted it.
To restore this behaviour, scatter calls to dropDbgValues around in places that used to delete chunks of dbg.values, for example during stripDebugInfo and in the code that deletes everything after an Unreachable instruction. DCE is another example.
The tests with --try... added to them are new scenariso where we can now correctly replicate the "normal" debug-info behaviour. Alas, there's no explicit test for the opt -strip-debug option though (in dbg.value mode or DPValue mode).