Page MenuHomePhabricator

[Instruction] Set metadata uses to undef on deletion
ClosedPublic

Authored by vsk on May 19 2020, 6:09 PM.

Details

Summary

Replace any extant metadata uses of a dying instruction with undef to
preserve debug info accuracy. Some alternatives include:

  • Treat Instruction like any other Value, and point its extant metadata uses to an empty ValueAsMetadata node. This makes extant dbg.value uses trivially dead (i.e. fair game for deletion in many passes), leading to stale dbg.values being in effect for too long.
  • Call salvageDebugInfoOrMarkUndef. Not needed to make instruction removal correct. OTOH results in wasted work in some common cases (e.g. when all instructions in a BasicBlock are deleted).

This came up while discussing some basic cases in
https://reviews.llvm.org/D80052.

Diff Detail

Event Timeline

vsk created this revision.May 19 2020, 6:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
vsk updated this revision to Diff 265108.May 19 2020, 6:11 PM

Remove accidentally included file.

vsk added a subscriber: debug-info.May 19 2020, 6:13 PM
Harbormaster completed remote builds in B57314: Diff 265108.
davide added a subscriber: davide.May 20 2020, 12:37 PM
vsk added a comment.May 20 2020, 5:02 PM

I forgot to add: a third alternative is to stop treating "dbg.value(metadata !{}, ...)" as trivially dead, but that seems not as nice, because then we'd have two representations for the same thing (c.f. "dbg.value(metadata <ty> undef, ...)".

Do you care to distinguish "the value was optimized out at this location", vs. "the value is uninitialized/poison at this location"?

jmorse accepted this revision.May 21 2020, 5:01 AM

LGTM; there may be a risk that non-debug metadata users of instructions are affected by this, but I wouldn't imagine the impact would be serious.

This revision is now accepted and ready to land.May 21 2020, 5:01 AM

NB, by fluke, this also fixes the recently reported https://bugs.llvm.org/show_bug.cgi?id=46006

vsk added a comment.May 21 2020, 2:17 PM

Do you care to distinguish "the value was optimized out at this location", vs. "the value is uninitialized/poison at this location"?

There are different representations for optimized-out vs. uninitialized source variables at the IR level, but this patch doesn't change the status quo.

IIUC there are two ways llvm represents uninitialized source variables at the IR level: 1) with a dbg.declare that points to an uninitialized alloca, or 2) by omission, e.g. by emitting a fragment dbg.value description of a variable that doesn't cover all its bits. By contrast, optimized-out variables should always be described as 'undef' (that's what this patch is trying to do).

I haven't really thought about the interaction of poison values with debug intrinsics at all. A vague notion I have is that poison can lead to DCE: if that's so, with this patch in place, there may be instances of "poisoned" variables appearing as <unavailable> in debug sessions, instead of appearing as misleading values.

This revision was automatically updated to reflect the committed changes.