Page MenuHomePhabricator

[mem2reg] Remove dbg.values describing contents of dead allocas
ClosedPublic

Authored by Orlando on Oct 20 2020, 10:31 AM.

Details

Reviewers
vsk
aprantl
rnk
Summary

This patch copies @vsk's fix to instcombine from D85555 over to mem2reg. The
motivation and rationale are exactly the same: When mem2reg 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.

As far as I can tell, prior to D80264 these dbg.value+derefs would have been
silently dropped instead of being made undef, so we're just returning to
previous behaviour with these patches.


Testing

llvm-lit llvm/test and ninja check-clang gave no unexpected failures. Added
3 tests, each of which covers a dbg.value deletion path in mem2reg:

mem2reg-promote-alloca-1.ll
mem2reg-promote-alloca-2.ll
mem2reg-promote-alloca-3.ll

The first is based on the dexter test inlining.c from D89543. This patch also
improves the debugging experience for loop.c from D89543, which suffers
similarly after arg promotion instead of inlining.


An example

Looking at one of the tests added as an example, mem2reg-promote-alloca-1.ll,
based on inlining.c from D89543. Starting with this source:

int g;
__attribute__((__always_inline__))
static void use(int* p) {
  g = *p;
}
void fun(int param) {
  use(&param);
}

After various transforms (including inlining) we get this IR. Note that the
dbg.values for param (!16) before the store and call were inserted by
instcombine's LowerDbgDeclare earlier in the pipeline.

define dso_local void @fun(i32 %param) !dbg !12 {
entry:
  %param.addr = alloca i32, align 4
  call void @llvm.dbg.value(metadata i32 %param, metadata !16, metadata !DIExpression()), !dbg !17
  store i32 %param, i32* %param.addr, align 4
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !16, metadata !DIExpression(DW_OP_deref)), !dbg !17
;; ### inline 'use' start ###
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  %0 = load i32, i32* %param.addr, align 4, !dbg !30
  store i32 %0, i32* @g, align 4, !dbg !31
;; ### inline 'use' end   ###
  ret void, !dbg !32
}

!16 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)
!22 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)

Then sroa runs, invoking mem2reg, and replaces the store+load with the stored
value and deletes the alloca. Without this patch, the`dbg.value+deref` becomes
undef:

define dso_local void @fun(i32 %param) !dbg !12 {
entry:
  call void @llvm.dbg.value(metadata i32 %param, metadata !16, metadata !DIExpression()), !dbg !17
  call void @llvm.dbg.value(metadata i32* undef, metadata !16, metadata !DIExpression(DW_OP_deref)), !dbg !17
;; ### inline 'use' start ###  
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  store i32 %param, i32* @g, align 4, !dbg !31
;; ### inline 'use' end   ###  
  ret void, !dbg !32
}

!16 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)
!22 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)

With this patch, the dbg.value+deref is instead removed:

define dso_local void @fun(i32 %param) !dbg !12 {
entry:
  call void @llvm.dbg.value(metadata i32 %param, metadata !16, metadata !DIExpression()), !dbg !17
;; ### inline 'use' start ###  
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  call void @llvm.dbg.value(metadata i32* %param.addr, metadata !22, metadata !DIExpression()), !dbg !28
  store i32 %param, i32* @g, align 4, !dbg !31
;; ### inline 'use' end   ###  
  ret void, !dbg !32
}

!16 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)
!22 = !DILocalVariable(name: "param", arg: 1, scope: !12, file: !6, line: 15, type: !7)

Diff Detail

Event Timeline

Orlando created this revision.Oct 20 2020, 10:31 AM
Orlando requested review of this revision.Oct 20 2020, 10:31 AM
vsk added a comment.Oct 20 2020, 12:28 PM

Thanks Orlando. We've discussed the pros & cons of deleting undef DW_OP_deref dbg.values in D85555. While there are cases where we do want to mark a location as 'undef' (say, after a dead store), on balance I think this is the better approach.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
104

Considering that this replaces a TinyPtrVector which "is specialized for cases where there are normally 0 or 1 element in a vector", perhaps we ought to reserve less memory?

683–684

nit, 'auto &DbgUsers'?

Orlando added a comment.EditedOct 21 2020, 1:42 AM

Hi @vsk, thanks for taking a look, I'll address your inline comments shortly.

On the topic of stale values after inlining+DSE: Could we teach the inliner to insert dbg.values in the same pattern as LowerDbgDeclare for parameters which are pointers to caller-local allocas? E.g. in the following example, we would have the inliner insert a dbg.value(%storevalue0xff, "param", DIExpression()) after the inlined store *p = 0xff. After the store is deleted we'd end up with a salvagedOrUndef dbg.value at the store site. wdyt?

int g;
__attribute__((__always_inline__))
static void use(int* p) {
  g = *p;
  *p = 0xff;  // After inlining, insert a dbg.value before this store, just like LowerDbgDeclare.
}
void fun(int param) {
  use(&param);
}
Orlando updated this revision to Diff 299607.Oct 21 2020, 2:20 AM
Orlando marked 2 inline comments as done.

Addressed @vsk's comments:
+ Resize SmallVector<...,8> -> SmallVector<...,1>
+ Rename Declares -> DbgUsers.

Orlando added inline comments.Oct 21 2020, 2:44 AM
llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
104

Makes sense, done. For added confidence, in a clang-3.4 RelWithDebInfo build using trunk I found that DbgUsers.size() is 0 or 1 for over 90% of AnalyzeAlloca calls, so 1 seems like a good choice.

vsk accepted this revision.Oct 21 2020, 11:54 AM

Thanks, lgtm.

Hi @vsk, thanks for taking a look, I'll address your inline comments shortly.

On the topic of stale values after inlining+DSE: Could we teach the inliner to insert dbg.values in the same pattern as LowerDbgDeclare for parameters which are pointers to caller-local allocas?

That sounds like a neat solution. I can't think of any issues with this, but it's probably worth a wider discussion (perhaps on a bugzilla PR or on llvm-dev).

This revision is now accepted and ready to land.Oct 21 2020, 11:54 AM

Apologies, in shortening the commit message I carelessly removed the review number. This was committed in fea067bdfde4.

Inliner improvement filed here https://llvm.org/47946

Orlando closed this revision.Oct 23 2020, 2:20 AM