[llvm] 00e2388 - [DebugInfo] Nerf placeDbgValues, with prejudice
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 9 04:52:46 PST 2019
Author: Jeremy Morse
Date: 2019-12-09T12:52:10Z
New Revision: 00e238896cd8ad3a7d715b8fb5f12a2e60af8a6f
URL: https://github.com/llvm/llvm-project/commit/00e238896cd8ad3a7d715b8fb5f12a2e60af8a6f
DIFF: https://github.com/llvm/llvm-project/commit/00e238896cd8ad3a7d715b8fb5f12a2e60af8a6f.diff
LOG: [DebugInfo] Nerf placeDbgValues, with prejudice
CodeGenPrepare::placeDebugValues moves variable location intrinsics to be
immediately after the Value they refer to. This makes tracking of locations
very easy; but it changes the order in which assignments appear to the
debugger, from the source programs order to the order in which the
optimised program computes values. This then leads to PR43986 and PR38754,
where variable locations that were in a conditional block are made
unconditional, which is highly misleading.
This patch adjusts placeDbgValues to only re-order variable location
intrinsics if they use a Value before it is defined, significantly reducing
the damage that it does. This is still not 100% safe, but the rest of
CodeGenPrepare needs polishing to correctly update debug info when
optimisations are performed to fully fix this.
This will probably break downstream debuginfo tests -- if the
instruction-stream position of variable location changes isn't the focus of
the test, an easy fix should be to manually apply placeDbgValues' behaviour
to the failing tests, moving dbg.value intrinsics next to SSA variable
definitions thus:
%foo = inst1
%bar = ...
%baz = ...
void call @llvm.dbg.value(metadata i32 %foo, ...
to
%foo = inst1
void call @llvm.dbg.value(metadata i32 %foo, ...
%bar = ...
%baz = ...
This should return your test to exercising whatever it was testing before.
Differential Revision: https://reviews.llvm.org/D58453
Added:
Modified:
llvm/include/llvm/CodeGen/MachineInstr.h
llvm/lib/CodeGen/CodeGenPrepare.cpp
llvm/test/DebugInfo/COFF/register-variables.ll
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/DW_AT_location-reference.ll
llvm/test/DebugInfo/X86/PR37234.ll
llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
llvm/test/tools/llvm-locstats/locstats.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index 55457aaa7830..a60244528df8 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1651,7 +1651,8 @@ class MachineInstr
/// Add all implicit def and use operands to this instruction.
void addImplicitDefUseOperands(MachineFunction &MF);
- /// Scan instructions following MI and collect any matching DBG_VALUEs.
+ /// Scan instructions immediately following MI and collect any matching
+ /// DBG_VALUEs.
void collectDebugValues(SmallVectorImpl<MachineInstr *> &DbgValues);
/// Find all DBG_VALUEs that point to the register def in this instruction
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index a041808199d4..a683fcf939d7 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -7230,42 +7230,46 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
return false;
}
-// llvm.dbg.value is far away from the value then iSel may not be able
-// handle it properly. iSel will drop llvm.dbg.value if it can not
-// find a node corresponding to the value.
+// A llvm.dbg.value may be using a value before its definition, due to
+// optimizations in this pass and others. Scan for such dbg.values, and rescue
+// them by moving the dbg.value to immediately after the value definition.
+// FIXME: Ideally this should never be necessary, and this has the potential
+// to re-order dbg.value intrinsics.
bool CodeGenPrepare::placeDbgValues(Function &F) {
bool MadeChange = false;
+ DominatorTree DT(F);
+
for (BasicBlock &BB : F) {
- Instruction *PrevNonDbgInst = nullptr;
for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {
Instruction *Insn = &*BI++;
DbgValueInst *DVI = dyn_cast<DbgValueInst>(Insn);
- // Leave dbg.values that refer to an alloca alone. These
- // intrinsics describe the address of a variable (= the alloca)
- // being taken. They should not be moved next to the alloca
- // (and to the beginning of the scope), but rather stay close to
- // where said address is used.
- if (!DVI || (DVI->getValue() && isa<AllocaInst>(DVI->getValue()))) {
- PrevNonDbgInst = Insn;
+ if (!DVI)
continue;
- }
Instruction *VI = dyn_cast_or_null<Instruction>(DVI->getValue());
- if (VI && VI != PrevNonDbgInst && !VI->isTerminator()) {
- // If VI is a phi in a block with an EHPad terminator, we can't insert
- // after it.
- if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
- continue;
- LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
- << *DVI << ' ' << *VI);
- DVI->removeFromParent();
- if (isa<PHINode>(VI))
- DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
- else
- DVI->insertAfter(VI);
- MadeChange = true;
- ++NumDbgValueMoved;
- }
+
+ if (!VI || VI->isTerminator())
+ continue;
+
+ // If VI is a phi in a block with an EHPad terminator, we can't insert
+ // after it.
+ if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
+ continue;
+
+ // If the defining instruction dominates the dbg.value, we do not need
+ // to move the dbg.value.
+ if (DT.dominates(VI, DVI))
+ continue;
+
+ LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
+ << *DVI << ' ' << *VI);
+ DVI->removeFromParent();
+ if (isa<PHINode>(VI))
+ DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
+ else
+ DVI->insertAfter(VI);
+ MadeChange = true;
+ ++NumDbgValueMoved;
}
}
return MadeChange;
diff --git a/llvm/test/DebugInfo/COFF/register-variables.ll b/llvm/test/DebugInfo/COFF/register-variables.ll
index e67c62a7c5b9..fe73f917e482 100644
--- a/llvm/test/DebugInfo/COFF/register-variables.ll
+++ b/llvm/test/DebugInfo/COFF/register-variables.ll
@@ -32,9 +32,6 @@
; ASM: #DEBUG_VALUE: f:p <- $esi
; ASM: callq getint
; ASM: [[after_getint:\.Ltmp.*]]:
-; ASM: #DEBUG_VALUE: a <- $eax
-; ASM: #DEBUG_VALUE: inlineinc:a <- $eax
-; ASM: #DEBUG_VALUE: c <- $eax
; ASM: testl %esi, %esi
; ASM: je .LBB0_2
; ASM: [[after_je:\.Ltmp.*]]:
@@ -45,16 +42,17 @@
; ASM: addl $1, %eax
; ASM: [[after_inc_eax:\.Ltmp.*]]:
; ASM: #DEBUG_VALUE: inlineinc:b <- $eax
-; ASM: #DEBUG_VALUE: b <- $eax
; ASM: addl $1, x(%rip)
; ASM: [[after_if:\.Ltmp.*]]:
; ASM: .LBB0_2: # %if.else
; ASM: #DEBUG_VALUE: f:p <- $esi
+; ASM: #DEBUG_VALUE: c <- $eax
; ASM: movl %eax, %ecx
; ASM: addq $32, %rsp
; ASM: popq %rsi
; ASM: [[func_end:\.Ltmp.*]]:
; ASM: jmp putint # TAILCALL
+; ASM: [[func_finished:\.Ltmp.*]]:
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "p"
@@ -62,18 +60,20 @@
; ASM: .cv_def_range [[p_ecx_esi]] [[func_end]], reg, 23
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "c"
-; ASM: .cv_def_range [[after_getint]] [[after_je]], reg, 17
+; ASM: .cv_def_range [[after_if]] [[func_finished]], reg, 17
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "a"
-; ASM: .cv_def_range [[after_getint]] [[after_inc_eax]], reg, 17
+; ASM: .cv_def_range [[after_je]] [[after_inc_eax]], reg, 17
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "b"
-; ASM: .cv_def_range [[after_inc_eax]] [[after_if]], reg, 17
+; ASM: .cv_def_range [[after_if]] [[after_if]], reg, 17
+
+; Note: "b" is a victim of tail de-duplication / branch folding.
; ASM: .short 4429 # Record kind: S_INLINESITE
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "a"
-; ASM: .cv_def_range [[after_getint]] [[after_inc_eax]], reg, 17
+; ASM: .cv_def_range [[after_je]] [[after_inc_eax]], reg, 17
; ASM: .short 4414 # Record kind: S_LOCAL
; ASM: .asciz "b"
; ASM: .cv_def_range [[after_inc_eax]] [[after_if]], reg, 17
@@ -116,9 +116,9 @@
; OBJ: DefRangeRegisterSym {
; OBJ: Register: EAX (0x11)
; OBJ: LocalVariableAddrRange {
-; OBJ: OffsetStart: .text+0xC
+; OBJ: OffsetStart: .text+0x1A
; OBJ: ISectStart: 0x0
-; OBJ: Range: 0x4
+; OBJ: Range: 0xC
; OBJ: }
; OBJ: }
; OBJ: LocalSym {
@@ -130,23 +130,9 @@
; OBJ: DefRangeRegisterSym {
; OBJ: Register: EAX (0x11)
; OBJ: LocalVariableAddrRange {
-; OBJ: OffsetStart: .text+0xC
+; OBJ: OffsetStart: .text+0x10
; OBJ: ISectStart: 0x0
-; OBJ: Range: 0x7
-; OBJ: }
-; OBJ: }
-; OBJ: LocalSym {
-; OBJ: Type: int (0x74)
-; OBJ: Flags [ (0x0)
-; OBJ: ]
-; OBJ: VarName: b
-; OBJ: }
-; OBJ: DefRangeRegisterSym {
-; OBJ: Register: EAX (0x11)
-; OBJ: MayHaveNoName: 0
-; OBJ: OffsetStart: .text+0x13
-; OBJ: ISectStart: 0x0
-; OBJ: Range: 0x7
+; OBJ: Range: 0x3
; OBJ: }
; OBJ: }
; OBJ: InlineSiteSym {
@@ -164,9 +150,9 @@
; OBJ: DefRangeRegisterSym {
; OBJ: Register: EAX (0x11)
; OBJ: LocalVariableAddrRange {
-; OBJ: OffsetStart: .text+0xC
+; OBJ: OffsetStart: .text+0x10
; OBJ: ISectStart: 0x0
-; OBJ: Range: 0x7
+; OBJ: Range: 0x3
; OBJ: }
; OBJ: }
; OBJ: LocalSym {
diff --git a/llvm/test/DebugInfo/NVPTX/debug-info.ll b/llvm/test/DebugInfo/NVPTX/debug-info.ll
index 9a572ff8ac74..d6cb8091d0fc 100644
--- a/llvm/test/DebugInfo/NVPTX/debug-info.ll
+++ b/llvm/test/DebugInfo/NVPTX/debug-info.ll
@@ -8388,8 +8388,8 @@ if.end: ; preds = %if.then, %entry
; CHECK-NEXT: .b8 37 // DW_AT_call_column
; CHECK-NEXT: .b8 43 // Abbrev [43] 0x2711:0x23 DW_TAG_inlined_subroutine
; CHECK-NEXT: .b32 9791 // DW_AT_abstract_origin
-; CHECK-NEXT: .b64 Ltmp10 // DW_AT_low_pc
-; CHECK-NEXT: .b64 Ltmp11 // DW_AT_high_pc
+; CHECK-NEXT: .b64 Ltmp9 // DW_AT_low_pc
+; CHECK-NEXT: .b64 Ltmp10 // DW_AT_high_pc
; CHECK-NEXT: .b8 12 // DW_AT_call_file
; CHECK-NEXT: .b8 8 // DW_AT_call_line
; CHECK-NEXT: .b8 5 // DW_AT_call_column
diff --git a/llvm/test/DebugInfo/X86/DW_AT_location-reference.ll b/llvm/test/DebugInfo/X86/DW_AT_location-reference.ll
index 43fde0bcc643..74b0afb2cf05 100644
--- a/llvm/test/DebugInfo/X86/DW_AT_location-reference.ll
+++ b/llvm/test/DebugInfo/X86/DW_AT_location-reference.ll
@@ -32,7 +32,9 @@
; CHECK: .debug_info contents:
; CHECK: DW_TAG_variable
; CHECK-NEXT: DW_AT_location [DW_FORM_sec_offset] (0x00000000
-; Check that the location contains only 2 ranges.
+; Check that the location contains only 4 ranges.
+; CHECK-NEXT: [0x{{[0-9a-f]*}}, 0x{{[0-9a-f]*}})
+; CHECK-NEXT: [0x{{[0-9a-f]*}}, 0x{{[0-9a-f]*}})
; CHECK-NEXT: [0x{{[0-9a-f]*}}, 0x{{[0-9a-f]*}})
; CHECK-NEXT: [0x{{[0-9a-f]*}}, 0x{{[0-9a-f]*}}){{.*}})
; CHECK-NEXT: DW_AT_name {{.*}} "x"
diff --git a/llvm/test/DebugInfo/X86/PR37234.ll b/llvm/test/DebugInfo/X86/PR37234.ll
index a0c8b91d624e..04005ca3fc58 100644
--- a/llvm/test/DebugInfo/X86/PR37234.ll
+++ b/llvm/test/DebugInfo/X86/PR37234.ll
@@ -20,9 +20,8 @@
; CHECK-LABEL: # %bb.{{.*}}:
; CHECK: #DEBUG_VALUE: main:aa <- 0
-; CHECK: #DEBUG_VALUE: main:aa <- $[[REG:[0-9a-z]+]]
; CHECK: .LBB0_1:
-; CHECK: #DEBUG_VALUE: main:aa <- $[[REG]]
+; CHECK: #DEBUG_VALUE: main:aa <- $[[REG:[0-9a-z]+]]
; CHECK: je .LBB0_4
; CHECK: # %bb.{{.*}}:
; CHECK: #DEBUG_VALUE: main:aa <- $[[REG]]
diff --git a/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll b/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
index a3a80e1c9b76..137316db6979 100644
--- a/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
+++ b/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
@@ -9,17 +9,12 @@
; memory instruction. Test that the one before does _not_ get updated (as that
; would either make it use-before-def or shift when the variable appears), and
; that the dbg.value after the memory instruction does get updated.
-;
-; Due to placeDbgValues, the dbg.values currently get shifted up a few
-; instructions.
define dso_local i8 @foo(i32 *%p, i32 %cond) !dbg !7 {
entry:
-; The first dbg.value of %arith, in the 'next' block, will be moved up here
-; by placeDbgValues,
+; There should be no dbg.values in this block.
; CHECK-LABEL: entry:
-; CHECK: dbg.value(metadata i8* %arith, metadata ![[DIVAR:[0-9]+]],
-; CHECK-SAME: metadata !DIExpression()
+; CHECK-NOT: dbg.value
%casted = bitcast i32 *%p to i8*
%arith = getelementptr i8, i8 *%casted, i32 3
%load1 = load i8, i8 *%arith
@@ -30,12 +25,14 @@ next:
; Address calcs should be duplicated into this block. One dbg.value should be
; updated, and the other should not.
; CHECK-LABEL: next:
-; CHECK: %[[CASTVAR:[0-9a-zA-Z]+]] = bitcast i32* %p to i8*
+; CHECK: dbg.value(metadata i8* %arith, metadata ![[DIVAR:[0-9]+]],
+; CHECK-SAME: metadata !DIExpression()
+; CHECK-NEXT: %[[CASTVAR:[0-9a-zA-Z]+]] = bitcast i32* %p to i8*
; CHECK-NEXT: %[[GEPVAR:[0-9a-zA-Z]+]] = getelementptr i8, i8* %[[CASTVAR]],
; CHECK-SAME: i64 3
+; CHECK-NEXT: %loaded = load i8, i8* %[[GEPVAR]]
; CHECK-NEXT: call void @llvm.dbg.value(metadata i8* %[[GEPVAR]],
; CHECK-SAME: metadata ![[DIVAR]],
-; CHECK-NEXT: %loaded = load i8, i8* %[[GEPVAR]]
call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
%loaded = load i8, i8 *%arith
call void @llvm.dbg.value(metadata i8 *%arith, metadata !12, metadata !DIExpression()), !dbg !14
diff --git a/llvm/test/tools/llvm-locstats/locstats.ll b/llvm/test/tools/llvm-locstats/locstats.ll
index 394d5129df7d..911abf5b711d 100644
--- a/llvm/test/tools/llvm-locstats/locstats.ll
+++ b/llvm/test/tools/llvm-locstats/locstats.ll
@@ -11,9 +11,9 @@
; LOCSTATS: 30-39% 0 0%
; LOCSTATS: 40-49% 1 11%
; LOCSTATS: 50-59% 1 11%
-; LOCSTATS: 60-69% 1 11%
+; LOCSTATS: 60-69% 0 0%
; LOCSTATS: 70-79% 0 0%
-; LOCSTATS: 80-89% 2 22%
+; LOCSTATS: 80-89% 3 33%
; LOCSTATS: 90-99% 1 11%
; LOCSTATS: 100% 2 22%
;
More information about the llvm-commits
mailing list