[llvm] 3419252 - [InstCombine] Remove dbg.values describing contents of dead allocas
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 22 10:00:25 PDT 2020
Author: Vedant Kumar
Date: 2020-10-22T10:00:13-07:00
New Revision: 3419252a792a1da459ef9659391b8a876bf6b630
URL: https://github.com/llvm/llvm-project/commit/3419252a792a1da459ef9659391b8a876bf6b630
DIFF: https://github.com/llvm/llvm-project/commit/3419252a792a1da459ef9659391b8a876bf6b630.diff
LOG: [InstCombine] Remove dbg.values describing contents of dead allocas
When InstCombine removes an alloca, it erases the dbg.{addr,declare}
instructions which refer to the alloca. It would be better to instead
remove all debug intrinsics which describe the contents of the dead
alloca, namely all dbg.value(<dead alloca>, ..., DW_OP_deref)'s.
This effectively undoes work performed in an InstCombine run earlier in
the pipeline by LowerDbgDeclare, which inserts DW_OP_deref dbg.values
before CallInst users of an alloca. The motivating example looks like:
```
define void @foo(i32 %0) {
%a = alloca i32 ; This alloca is erased.
store i32 %0, i32* %a
dbg.value(i32 %0, "arg0") ; This dbg.value survives.
dbg.value(i32* %a, "arg0", DW_OP_deref)
call void @trivially_inlinable_no_op(i32* %a)
ret void
}
```
If the DW_OP_deref dbg.value is not erased, it becomes dbg.value(undef)
after inlining, making "arg0" unavailable. But we already have dbg.value
descriptions of the alloca's value (from LowerDbgDeclare), so the
DW_OP_deref dbg.value cannot serve its purpose of describing an
initialization of the alloca by some callee. It invalidates other useful
dbg.values, causing large gaps in location coverage, so we should delete
it (even though doing so may cause stale dbg.values to appear, if
there's a dead store to `%a` in @trivially_inlinable_no_op).
OTOH, it wouldn't be correct to delete all dbg.value descriptions of an
alloca. Note that it's possible to describe a variable that takes on
different pointer values, e.g.:
```
void use(int *);
void t(int a, int b) {
int *local = &a; // dbg.value(i32* %a.addr, "local")
local = &b; // dbg.value(i32* undef, "local")
use(&a); // (note: %b.addr is optimized out)
local = &a; // dbg.value(i32* %a.addr, "local")
}
```
In this example, the alloca for "b" is erased, but we need to describe
the value of "local" as <unavailable> before the call to "use". This
prevents "local" from appearing to be equal to "&a" at the callsite.
rdar://66592859
Differential Revision: https://reviews.llvm.org/D85555
Added:
llvm/test/Transforms/InstCombine/erase-dbg-values-at-dead-alloc-site.ll
Modified:
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 0b9f8667746b..3b38ed2d6604 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2589,10 +2589,10 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) {
// If we are removing an alloca with a dbg.declare, insert dbg.value calls
// before each store.
- TinyPtrVector<DbgVariableIntrinsic *> DIIs;
+ SmallVector<DbgVariableIntrinsic *, 8> DVIs;
std::unique_ptr<DIBuilder> DIB;
if (isa<AllocaInst>(MI)) {
- DIIs = FindDbgAddrUses(&MI);
+ findDbgUsers(DVIs, &MI);
DIB.reset(new DIBuilder(*MI.getModule(), /*AllowUnresolved=*/false));
}
@@ -2626,8 +2626,9 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) {
ConstantInt::get(Type::getInt1Ty(C->getContext()),
C->isFalseWhenEqual()));
} else if (auto *SI = dyn_cast<StoreInst>(I)) {
- for (auto *DII : DIIs)
- ConvertDebugDeclareToDebugValue(DII, SI, *DIB);
+ for (auto *DVI : DVIs)
+ if (DVI->isAddressOfVariable())
+ ConvertDebugDeclareToDebugValue(DVI, SI, *DIB);
} else {
// Casts, GEP, or anything else: we're about to delete this instruction,
// so it can not have any valid uses.
@@ -2644,8 +2645,31 @@ Instruction *InstCombinerImpl::visitAllocSite(Instruction &MI) {
None, "", II->getParent());
}
- for (auto *DII : DIIs)
- eraseInstFromFunction(*DII);
+ // Remove debug intrinsics which describe the value contained within the
+ // alloca. In addition to removing dbg.{declare,addr} which simply point to
+ // the alloca, remove dbg.value(<alloca>, ..., DW_OP_deref)'s as well, e.g.:
+ //
+ // ```
+ // define void @foo(i32 %0) {
+ // %a = alloca i32 ; Deleted.
+ // store i32 %0, i32* %a
+ // dbg.value(i32 %0, "arg0") ; Not deleted.
+ // dbg.value(i32* %a, "arg0", DW_OP_deref) ; Deleted.
+ // call void @trivially_inlinable_no_op(i32* %a)
+ // ret void
+ // }
+ // ```
+ //
+ // This may not be required if we stop describing the contents of allocas
+ // using dbg.value(<alloca>, ..., DW_OP_deref), but we currently do this in
+ // the LowerDbgDeclare utility.
+ //
+ // If there is a dead store to `%a` in @trivially_inlinable_no_op, the
+ // "arg0" dbg.value may be stale after the call. However, failing to remove
+ // the DW_OP_deref dbg.value causes large gaps in location coverage.
+ for (auto *DVI : DVIs)
+ if (DVI->isAddressOfVariable() || DVI->getExpression()->startsWithDeref())
+ DVI->eraseFromParent();
return eraseInstFromFunction(MI);
}
diff --git a/llvm/test/Transforms/InstCombine/erase-dbg-values-at-dead-alloc-site.ll b/llvm/test/Transforms/InstCombine/erase-dbg-values-at-dead-alloc-site.ll
new file mode 100644
index 000000000000..ceb63e600128
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/erase-dbg-values-at-dead-alloc-site.ll
@@ -0,0 +1,76 @@
+; RUN: opt -S -instcombine %s | FileCheck %s -check-prefix=RUN-ONCE
+
+; This example was reduced from a test case in which InstCombine ran at least
+; twice:
+; - The first InstCombine run converted dbg.declares to dbg.values using the
+; LowerDbgDeclare utility. This produced a dbg.value(i32* %2, DW_OP_deref)
+; (this happens when the contents of an alloca are passed by-value), and a
+; dbg.value(i32 %0) (due to the store of %0 into the alloca).
+; - The second InstCombine run deleted the alloca (%2).
+; Check that the DW_OP_deref dbg.value is deleted, just like a dbg.declare would
+; be.
+;
+; RUN-ONCE-LABEL: @t1(
+; RUN-ONCE-NEXT: llvm.dbg.value(metadata i32 %0, metadata [[t1_arg0:![0-9]+]], metadata !DIExpression())
+; RUN-ONCE-NEXT: llvm.dbg.value(metadata i32* undef, metadata [[t1_fake_ptr:![0-9]+]], metadata !DIExpression())
+; RUN-ONCE-NEXT: ret void
+define void @t1(i32) !dbg !9 {
+ %2 = alloca i32, align 4
+ store i32 %0, i32* %2, align 4
+ call void @llvm.dbg.value(metadata i32 %0, metadata !14, metadata !DIExpression()), !dbg !15
+ call void @llvm.dbg.value(metadata i32* %2, metadata !14, metadata !DIExpression(DW_OP_deref)), !dbg !15
+ call void @llvm.dbg.value(metadata i32* %2, metadata !20, metadata !DIExpression()), !dbg !15
+ ret void
+}
+
+; This example is closer to an end-to-end test: the IR looks like it could have
+; been produced by a frontend compiling at -O0.
+;
+; Here's what happens:
+; 1) We run InstCombine. This puts a dbg.value(i32* %x.addr, DW_OP_deref)
+; before the call to @use, and a dbg.value(i32 %x) after the store.
+; 2) We inline @use.
+; 3) We run InstCombine again. The alloca %x.addr is erased. We should just get
+; dbg.value(i32 %x). There should be no leftover dbg.value(metadata i32*
+; undef).
+;
+;;; define void @use(i32* %addr) alwaysinline { ret void }
+;;; define void @t2(i32 %x) !dbg !17 {
+;;; %x.addr = alloca i32, align 4
+;;; store i32 %x, i32* %x.addr, align 4
+;;; call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !18, metadata !DIExpression()), !dbg !19
+;;; call void @use(i32* %x.addr)
+;;; ret void
+;;; }
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!0, !1, !2, !3, !4}
+!llvm.dbg.cu = !{!5}
+!llvm.ident = !{!8}
+
+; RUN-ONCE: [[t1_arg0]] = !DILocalVariable(name: "a"
+; RUN-ONCE: [[t1_fake_ptr]] = !DILocalVariable(name: "fake_ptr"
+
+!0 = !{i32 2, !"SDK Version", [2 x i32] [i32 10, i32 14]}
+!1 = !{i32 2, !"Dwarf Version", i32 4}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !{i32 1, !"wchar_size", i32 4}
+!4 = !{i32 7, !"PIC Level", i32 2}
+!5 = distinct !DICompileUnit(language: DW_LANG_C99, file: !6, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !7, nameTableKind: GNU)
+!6 = !DIFile(filename: "-", directory: "/")
+!7 = !{}
+!8 = !{!""}
+!9 = distinct !DISubprogram(name: "t1", scope: !10, file: !10, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !5, retainedNodes: !7)
+!10 = !DIFile(filename: "<stdin>", directory: "/")
+!11 = !DISubroutineType(types: !12)
+!12 = !{null, !13}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocalVariable(name: "a", arg: 1, scope: !9, file: !10, line: 1, type: !13)
+!15 = !DILocation(line: 1, column: 13, scope: !9)
+!16 = !DILocation(line: 1, column: 17, scope: !9)
+!17 = distinct !DISubprogram(name: "t2", scope: !10, file: !10, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !5, retainedNodes: !7)
+!18 = !DILocalVariable(name: "x", arg: 1, scope: !17, file: !10, line: 1, type: !13)
+!19 = !DILocation(line: 1, column: 1, scope: !17)
+!20 = !DILocalVariable(name: "fake_ptr", scope: !9, file: !10, line: 1, type: !13)
More information about the llvm-commits
mailing list