[llvm] r353824 - [DebugInfo] Don't salvage load operations (PR40628).
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 12 02:54:30 PST 2019
Author: jmorse
Date: Tue Feb 12 02:54:30 2019
New Revision: 353824
URL: http://llvm.org/viewvc/llvm-project?rev=353824&view=rev
Log:
[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
Added:
llvm/trunk/test/DebugInfo/Generic/pr40628.ll
Modified:
llvm/trunk/lib/Transforms/Utils/Local.cpp
llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll
llvm/trunk/test/Transforms/GVN/fence.ll
llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll
Modified: llvm/trunk/lib/Transforms/Utils/Local.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/Local.cpp?rev=353824&r1=353823&r2=353824&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/Local.cpp Tue Feb 12 02:54:30 2019
@@ -1714,9 +1714,9 @@ DIExpression *llvm::salvageDebugInfoImpl
// TODO: Salvage constants from each kind of binop we know about.
return nullptr;
}
- } else if (isa<LoadInst>(&I)) {
- // Rewrite the load into DW_OP_deref.
- return DIExpression::prepend(SrcDIExpr, DIExpression::WithDeref);
+ // *Not* to do: we should not attempt to salvage load instructions,
+ // because the validity and lifetime of a dbg.value containing
+ // DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
}
return nullptr;
}
Added: llvm/trunk/test/DebugInfo/Generic/pr40628.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/Generic/pr40628.ll?rev=353824&view=auto
==============================================================================
--- llvm/trunk/test/DebugInfo/Generic/pr40628.ll (added)
+++ llvm/trunk/test/DebugInfo/Generic/pr40628.ll Tue Feb 12 02:54:30 2019
@@ -0,0 +1,56 @@
+; RUN: opt -early-cse -S %s -o - | FileCheck %s
+
+; PR40628: The first load below is determined to be redundant by EarlyCSE.
+; During salvaging, the corresponding dbg.value could have a DW_OP_deref used
+; in its DIExpression to represent the redundant operation -- however LLVM
+; cannot currently determine that the subsequent store should terminate the
+; variables location range. A debugger would display zero for the "redundant"
+; variable after stepping onto the return instruction.
+
+; Test that the load being removed results in the corresponding dbg.value
+; being assigned the 'undef' value.
+
+; CHECK: @foo
+; CHECK-NEXT: dbg.value(metadata i32 undef, metadata ![[DEADVAR:[0-9]+]],
+; CHECK-NEXT: load
+; CHECK-NEXT: dbg.value(metadata i32 %{{[0-9]+}}, metadata ![[LIVEVAR:[0-9]+]],
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+
+; CHECK: ![[DEADVAR]] = !DILocalVariable(name: "redundant",
+; CHECK: ![[LIVEVAR]] = !DILocalVariable(name: "loaded",
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local i32 @foo(i32*) !dbg !7 {
+ %2 = load i32, i32* %0, align 4, !dbg !23
+ call void @llvm.dbg.value(metadata i32 %2, metadata !16, metadata !DIExpression()), !dbg !23
+ %3 = load i32, i32* %0, align 4, !dbg !23
+ call void @llvm.dbg.value(metadata i32 %3, metadata !17, metadata !DIExpression()), !dbg !23
+ store i32 0, i32* %0, align 4, !dbg !23
+ ret i32 %3, !dbg !23
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "pr40628.c", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!12 = !{!16, !17}
+!16 = !DILocalVariable(name: "redundant", scope: !7, file: !1, line: 4, type: !10)
+!17 = !DILocalVariable(name: "loaded", scope: !7, file: !1, line: 5, type: !10)
+!23 = !DILocation(line: 4, column: 7, scope: !7)
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll?rev=353824&r1=353823&r2=353824&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll (original)
+++ llvm/trunk/test/Transforms/DeadStoreElimination/debuginfo.ll Tue Feb 12 02:54:30 2019
@@ -6,23 +6,21 @@ declare noalias i8* @malloc(i32)
declare void @test_f()
-define i32* @test_salvage() {
+define i32* @test_salvage(i32 %arg) {
; Check that all four original local variables have their values preserved.
-; CHECK-LABEL: @test_salvage()
+; CHECK-LABEL: @test_salvage(
; CHECK-NEXT: malloc
; CHECK-NEXT: @llvm.dbg.value(metadata i8* %p, metadata ![[p:.*]], metadata !DIExpression())
; CHECK-NEXT: bitcast
; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[P:.*]], metadata !DIExpression())
-; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_deref))
-; 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))
+; CHECK-NEXT: @llvm.dbg.value(metadata i32 %arg, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value))
; CHECK-NEXT: call void @test_f()
; CHECK-NEXT: store i32 0, i32* %P
%p = tail call i8* @malloc(i32 4)
%P = bitcast i8* %p to i32*
- %DEAD = load i32, i32* %P
- %DEAD2 = add i32 %DEAD, 1
- store i32 %DEAD2, i32* %P
+ %DEAD = add i32 %arg, 1
+ store i32 %DEAD, i32* %P
call void @test_f()
store i32 0, i32* %P
ret i32* %P
@@ -31,4 +29,3 @@ define i32* @test_salvage() {
; CHECK: ![[p]] = !DILocalVariable(name: "1"
; CHECK: ![[P]] = !DILocalVariable(name: "2"
; CHECK: ![[DEAD]] = !DILocalVariable(name: "3"
-; CHECK: ![[DEAD2]] = !DILocalVariable(name: "4"
Modified: llvm/trunk/test/Transforms/GVN/fence.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/fence.ll?rev=353824&r1=353823&r2=353824&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/fence.ll (original)
+++ llvm/trunk/test/Transforms/GVN/fence.ll Tue Feb 12 02:54:30 2019
@@ -1,4 +1,4 @@
-; RUN: opt -S -debugify -basicaa -gvn < %s | FileCheck %s
+; RUN: opt -S -basicaa -gvn < %s | FileCheck %s
@a = external constant i32
; We can value forward across the fence since we can (semantically)
@@ -18,10 +18,8 @@ define i32 @test(i32* %addr.i) {
; Same as above
define i32 @test2(i32* %addr.i) {
; CHECK-LABEL: @test2
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a:![0-9]+]], metadata !DIExpression(DW_OP_deref))
; CHECK-NEXT: fence
; CHECK-NOT: load
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a2:![0-9]+]], metadata !DIExpression(DW_OP_deref))
; CHECK: ret
%a = load i32, i32* %addr.i, align 4
fence release
@@ -88,6 +86,3 @@ define i32 @test4(i32* %addr) {
; }
; Given we chose to forward across the release fence, we clearly can't forward
; across the acquire fence as well.
-
-; CHECK: [[var_a]] = !DILocalVariable
-; CHECK-NEXT: [[var_a2]] = !DILocalVariable
Modified: llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll?rev=353824&r1=353823&r2=353824&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll (original)
+++ llvm/trunk/test/Transforms/InstCombine/debuginfo-dce.ll Tue Feb 12 02:54:30 2019
@@ -23,6 +23,9 @@ target triple = "x86_64-apple-macosx10.1
%struct.entry = type { %struct.entry* }
+; This salvage can't currently occur safely (PR40628), however if/when that's
+; ever fixed, then this is definitely a piece of test coverage that should
+; be maintained.
define void @salvage_load(%struct.entry** %queue) local_unnamed_addr #0 !dbg !14 {
entry:
%im_not_dead = alloca %struct.entry*
@@ -31,8 +34,7 @@ entry:
call void @llvm.dbg.value(metadata %struct.entry* %1, metadata !18, metadata !20), !dbg !19
; CHECK: define void @salvage_load
; CHECK-NEXT: entry:
-; CHECK-NEXT: call void @llvm.dbg.value(metadata %struct.entry** %queue,
-; CHECK-SAME: metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 0))
+; CHECK-NOT: dbg.value
store %struct.entry* %1, %struct.entry** %im_not_dead, align 8
ret void, !dbg !21
}
More information about the llvm-commits
mailing list