This is an archive of the discontinued LLVM Phabricator instance.

Make salvageDebugInfo of casts work for dbg.declare and dbg.addr
ClosedPublic

Authored by rnk on Nov 14 2017, 11:11 AM.

Details

Summary

Instcombine (and probably other passes) sometimes want to change the
type of an alloca. To do this, they generally create a new alloca with
the desired type, create a bitcast to make the new pointer type match
the old pointer type, replace all uses with the cast, and then simplify
the casts. We already knew how to salvage dbg.value instructions when
removing casts, but we can extend it to cover dbg.addr and dbg.declare.

Fixes a debug info quality issue uncovered in Chromium in
http://crbug.com/784609

Event Timeline

rnk created this revision.Nov 14 2017, 11:11 AM
aprantl accepted this revision.Nov 14 2017, 11:19 AM

Nice!

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
150

are there metadata users besides debug info? Would findDbgValues() be more readable?

This revision is now accepted and ready to land.Nov 14 2017, 11:19 AM
rnk added inline comments.Nov 14 2017, 11:26 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
150

I can't think of any non-debug info uses. I did it this way because I wanted it to handle dbg.value, dbg.declare, and dbg.addr uniformly. This matter in situations where the value of a variable is the address of a variable (my favorite test case):

struct Foo PromoteMe = *p;
struct Foo *addr = &PromoteMe; // dbg.value(&PromoteMe)

An alternative way of looking at this bug is that it is a failure of salvageDebugInfo to handle dbg.declare. Actually, I wonder if I can update its use of findDbgValues to use this pattern and see if that fixes the bug more generally.

vsk added inline comments.Nov 14 2017, 11:42 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
150

In this case, how would salvageDebugInfo() know to attach the debug info from the old alloca to the new one? Would you pass a 'destination' Instruction to salvageDebugInfo?

rnk updated this revision to Diff 122890.Nov 14 2017, 11:52 AM
  • rewrite with salvageDebugInfo
rnk added a comment.Nov 14 2017, 11:52 AM

I rewrote this with salvageDebugInfo, take another look.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
150

Without this code, the tmpcast instruction below is used as the replacement. Later, instcombine combines that cast with another cast, and we lose the dbg.declare. If we can make salvageDebugInfo work on deleted casts, we can fix this bug more generally.

rnk retitled this revision from [InstCombine] Replace metadata alloca uses without a cast to Make salvageDebugInfo of casts work for dbg.declare and dbg.addr.Nov 14 2017, 11:54 AM
rnk edited the summary of this revision. (Show Details)
vsk added inline comments.Nov 14 2017, 1:10 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
150

Gotcha, thanks for explaining. I see how this is hooked up via eraseInstFromFunction() now.

aprantl accepted this revision.Nov 14 2017, 1:19 PM

That looks even better!

llvm/lib/Transforms/Utils/Local.cpp
1296 ↗(On Diff #122890)

Is there a point in keeping findDbgValues or should we just rename it?
It seems like every case where currently use it would also apply to dbg.addr.

1387 ↗(On Diff #122890)

By the way, can you find a way to turn this into a switch? I don't like this massive if-cascade, but it wasn't immediately obvious to me how to convert it.

rnk added inline comments.Nov 14 2017, 1:43 PM
llvm/lib/Transforms/Utils/Local.cpp
1296 ↗(On Diff #122890)

I felt there were too many callers for this patch. There are some users like loop passes that probably only want dbg.values for updating induction variable expressions that wouldn't apply to local variable addresses. Similarly, the GEP case in salvageDebugInfo applies DW_OP_stack_value which didn't feel right to add to the expression of a dbg.declare.

1387 ↗(On Diff #122890)

I'll take a look in a follow-up.

This revision was automatically updated to reflect the committed changes.