Skip to content

Commit b33a5c7

Browse files
jmorsejmorse
committedFeb 12, 2019
[DebugInfo] Don't salvage load operations (PR40628).
Salvaging a redundant load instruction into a debug expression hides a memory read from optimisation passes. Passes that alter memory behaviour (such as LICM promoting memory to a register) aren't aware of these debug memory reads and leave them unaltered, making the debug variable location point somewhere unsafe. Teaching passes to know about these debug memory reads would be challenging and probably incomplete. Finding dbg.value instructions that need to be fixed would likely be computationally expensive too, as more analysis would be required. It's better to not generate debug-memory-reads instead, alas. Changed tests: * DeadStoreElim: test for salvaging of intermediate operations contributing to the dead store, instead of salvaging of the redundant load, * GVN: remove debuginfo behaviour checks completely, this behaviour is still covered by other tests, * InstCombine: don't test for salvaged loads, we're removing that behaviour. Differential Revision: https://reviews.llvm.org/D57962 llvm-svn: 353824
·
llvmorg-21-initllvmorg-9.0.0-rc1
1 parent bbd2f97 commit b33a5c7

File tree

5 files changed

+69
-19
lines changed

5 files changed

+69
-19
lines changed
 

‎llvm/lib/Transforms/Utils/Local.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -1714,9 +1714,9 @@ DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
17141714
// TODO: Salvage constants from each kind of binop we know about.
17151715
return nullptr;
17161716
}
1717-
} else if (isa<LoadInst>(&I)) {
1718-
// Rewrite the load into DW_OP_deref.
1719-
return DIExpression::prepend(SrcDIExpr, DIExpression::WithDeref);
1717+
// *Not* to do: we should not attempt to salvage load instructions,
1718+
// because the validity and lifetime of a dbg.value containing
1719+
// DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
17201720
}
17211721
return nullptr;
17221722
}

‎llvm/test/DebugInfo/Generic/pr40628.ll

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
; RUN: opt -early-cse -S %s -o - | FileCheck %s
2+
3+
; PR40628: The first load below is determined to be redundant by EarlyCSE.
4+
; During salvaging, the corresponding dbg.value could have a DW_OP_deref used
5+
; in its DIExpression to represent the redundant operation -- however LLVM
6+
; cannot currently determine that the subsequent store should terminate the
7+
; variables location range. A debugger would display zero for the "redundant"
8+
; variable after stepping onto the return instruction.
9+
10+
; Test that the load being removed results in the corresponding dbg.value
11+
; being assigned the 'undef' value.
12+
13+
; CHECK: @foo
14+
; CHECK-NEXT: dbg.value(metadata i32 undef, metadata ![[DEADVAR:[0-9]+]],
15+
; CHECK-NEXT: load
16+
; CHECK-NEXT: dbg.value(metadata i32 %{{[0-9]+}}, metadata ![[LIVEVAR:[0-9]+]],
17+
; CHECK-NEXT: store
18+
; CHECK-NEXT: ret
19+
20+
; CHECK: ![[DEADVAR]] = !DILocalVariable(name: "redundant",
21+
; CHECK: ![[LIVEVAR]] = !DILocalVariable(name: "loaded",
22+
23+
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
24+
target triple = "x86_64-unknown-linux-gnu"
25+
26+
define dso_local i32 @foo(i32*) !dbg !7 {
27+
%2 = load i32, i32* %0, align 4, !dbg !23
28+
call void @llvm.dbg.value(metadata i32 %2, metadata !16, metadata !DIExpression()), !dbg !23
29+
%3 = load i32, i32* %0, align 4, !dbg !23
30+
call void @llvm.dbg.value(metadata i32 %3, metadata !17, metadata !DIExpression()), !dbg !23
31+
store i32 0, i32* %0, align 4, !dbg !23
32+
ret i32 %3, !dbg !23
33+
}
34+
35+
declare void @llvm.dbg.value(metadata, metadata, metadata)
36+
37+
!llvm.dbg.cu = !{!0}
38+
!llvm.module.flags = !{!3, !4, !5}
39+
!llvm.ident = !{!6}
40+
41+
!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
42+
!1 = !DIFile(filename: "pr40628.c", directory: ".")
43+
!2 = !{}
44+
!3 = !{i32 2, !"Dwarf Version", i32 4}
45+
!4 = !{i32 2, !"Debug Info Version", i32 3}
46+
!5 = !{i32 1, !"wchar_size", i32 4}
47+
!6 = !{!"clang"}
48+
!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
49+
!8 = !DISubroutineType(types: !9)
50+
!9 = !{!10, !11}
51+
!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
52+
!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
53+
!12 = !{!16, !17}
54+
!16 = !DILocalVariable(name: "redundant", scope: !7, file: !1, line: 4, type: !10)
55+
!17 = !DILocalVariable(name: "loaded", scope: !7, file: !1, line: 5, type: !10)
56+
!23 = !DILocation(line: 4, column: 7, scope: !7)

‎llvm/test/Transforms/DeadStoreElimination/debuginfo.ll

+5-8
Original file line numberDiff line numberDiff line change
@@ -6,23 +6,21 @@ declare noalias i8* @malloc(i32)
66

77
declare void @test_f()
88

9-
define i32* @test_salvage() {
9+
define i32* @test_salvage(i32 %arg) {
1010
; Check that all four original local variables have their values preserved.
11-
; CHECK-LABEL: @test_salvage()
11+
; CHECK-LABEL: @test_salvage(
1212
; CHECK-NEXT: malloc
1313
; CHECK-NEXT: @llvm.dbg.value(metadata i8* %p, metadata ![[p:.*]], metadata !DIExpression())
1414
; CHECK-NEXT: bitcast
1515
; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[P:.*]], metadata !DIExpression())
16-
; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_deref))
17-
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD2:.*]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 1, DW_OP_stack_value))
16+
; CHECK-NEXT: @llvm.dbg.value(metadata i32 %arg, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value))
1817
; CHECK-NEXT: call void @test_f()
1918
; CHECK-NEXT: store i32 0, i32* %P
2019

2120
%p = tail call i8* @malloc(i32 4)
2221
%P = bitcast i8* %p to i32*
23-
%DEAD = load i32, i32* %P
24-
%DEAD2 = add i32 %DEAD, 1
25-
store i32 %DEAD2, i32* %P
22+
%DEAD = add i32 %arg, 1
23+
store i32 %DEAD, i32* %P
2624
call void @test_f()
2725
store i32 0, i32* %P
2826
ret i32* %P
@@ -31,4 +29,3 @@ define i32* @test_salvage() {
3129
; CHECK: ![[p]] = !DILocalVariable(name: "1"
3230
; CHECK: ![[P]] = !DILocalVariable(name: "2"
3331
; CHECK: ![[DEAD]] = !DILocalVariable(name: "3"
34-
; CHECK: ![[DEAD2]] = !DILocalVariable(name: "4"

‎llvm/test/Transforms/GVN/fence.ll

+1-6
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: opt -S -debugify -basicaa -gvn < %s | FileCheck %s
1+
; RUN: opt -S -basicaa -gvn < %s | FileCheck %s
22

33
@a = external constant i32
44
; We can value forward across the fence since we can (semantically)
@@ -18,10 +18,8 @@ define i32 @test(i32* %addr.i) {
1818
; Same as above
1919
define i32 @test2(i32* %addr.i) {
2020
; CHECK-LABEL: @test2
21-
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a:![0-9]+]], metadata !DIExpression(DW_OP_deref))
2221
; CHECK-NEXT: fence
2322
; CHECK-NOT: load
24-
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a2:![0-9]+]], metadata !DIExpression(DW_OP_deref))
2523
; CHECK: ret
2624
%a = load i32, i32* %addr.i, align 4
2725
fence release
@@ -88,6 +86,3 @@ define i32 @test4(i32* %addr) {
8886
; }
8987
; Given we chose to forward across the release fence, we clearly can't forward
9088
; across the acquire fence as well.
91-
92-
; CHECK: [[var_a]] = !DILocalVariable
93-
; CHECK-NEXT: [[var_a2]] = !DILocalVariable

‎llvm/test/Transforms/InstCombine/debuginfo-dce.ll

+4-2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ target triple = "x86_64-apple-macosx10.12.0"
2323

2424
%struct.entry = type { %struct.entry* }
2525

26+
; This salvage can't currently occur safely (PR40628), however if/when that's
27+
; ever fixed, then this is definitely a piece of test coverage that should
28+
; be maintained.
2629
define void @salvage_load(%struct.entry** %queue) local_unnamed_addr #0 !dbg !14 {
2730
entry:
2831
%im_not_dead = alloca %struct.entry*
@@ -31,8 +34,7 @@ entry:
3134
call void @llvm.dbg.value(metadata %struct.entry* %1, metadata !18, metadata !20), !dbg !19
3235
; CHECK: define void @salvage_load
3336
; CHECK-NEXT: entry:
34-
; CHECK-NEXT: call void @llvm.dbg.value(metadata %struct.entry** %queue,
35-
; CHECK-SAME: metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 0))
37+
; CHECK-NOT: dbg.value
3638
store %struct.entry* %1, %struct.entry** %im_not_dead, align 8
3739
ret void, !dbg !21
3840
}

0 commit comments

Comments
 (0)
Please sign in to comment.