This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Describe the size of value referred to by a DBG_PHI
ClosedPublic

Authored by jmorse on Apr 21 2022, 9:27 AM.

Details

Summary

This patch records more information about stack slots in DBG_PHI instructions. To recap, DBG_PHIs are produced wherever there used to be a PHI with an instruction number attached, to record where the value gets defined when we leave SSA-form. Normally they refer to registers, but sometimes they can refer to stack locations -- PHIs can happen on the stack just as well as in physregs. A flaw in this is that we don't document the size of the value on the stack: it could be a single byte, or it could be a 16 byte vector. That then leads to InstrRefBasedLDV guessing, and sometimes getting it wrong, see D123599#3448767.

To fix this, this patch adds an extra operand to DBG_PHIs if they refer to a stack slot, containing the bit size of the value being merged. That allows us to disambiguate the original size of the value, from the size of the stack slot. We're free to add extra operands to debug instructions as they're meta-instructions, and we shouldn't need to worry any other piece of code rewriting DBG_PHIs to stack locations, because they should only ever refer to physregs or stack slots anyway. (As they're only generated by LiveDebugVariables and function arguments).

Two tests are added to cover the useful cases: if we coalesce a small 16 bit vreg where a PHI happens into a larger 32 bit vreg, and then put it on the stack, we should issue a DBG_PHI for the stack location and record that the value is 16 bits wide. That's done in phi-on-stack-coalesced.mir . dbg-phis-in-ldv2.mir checks that LiveDebugValues correctly interprets the bit size and selects the correct register location, with the correct size. AKA, it picks the right subregister for a value.

phi-on-stack-coalesced2.mir tests for an edge case: what happens if a DBG_PHI refers to a stack slot, but the value is not at a zero offset of the stack slot? Right now we just ignore that and get the location wrong -- with this patch, we just don't emit a DBG_PHI, causing any variable that refers to it to become optimised out. Future work is would be to extend DBG_PHI with an offset as well as a size, however I'm not confident that LLVM will ever produce code that would make use of this.

Diff Detail

Event Timeline

jmorse created this revision.Apr 21 2022, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 9:27 AM
jmorse requested review of this revision.Apr 21 2022, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 9:27 AM

LGTM

llvm/lib/CodeGen/LiveDebugVariables.cpp
1879

nit: Is that LLVM_DEBUG(...) clang-formatted? The bot isn't complaining but it looks unusual to me so just wanted to check.

aprantl added inline comments.
llvm/test/DebugInfo/MIR/InstrRef/dbg-phis-in-ldv2.mir
52

A lot of these DILocations seem to be unused?

jmorse marked an inline comment as done.Apr 25 2022, 4:58 AM
jmorse added inline comments.
llvm/test/DebugInfo/MIR/InstrRef/dbg-phis-in-ldv2.mir
52

I'll trim them out when landing shortly.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 25 2022, 5:42 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
jmorse marked an inline comment as done.