Page MenuHomePhabricator

[DebugInfo] Separate DbgValueInst and DIExpression logic in salvageDebugInfo for reusability.
ClosedPublic

Authored by jmorse on Mon, Feb 4, 9:26 AM.

Details

Summary

This patch refactors the logic for folding/salvaging the operation of an instruction into a DIExpression. The rationale behind this is to allow salvage operations of debug info that is not represented by a dbg.value intrinsic, specifically in D57694. There should be no functional change. (There might also be better ways of achieving D57694 without this change, input appreciated).

This is a fairly simple separation of the segment of salvageDebugInfo that gets/sets fields of DbgVariableIntrinsic insts, and the segment that determines a list of opcodes to add to the DIExpression. This builds in the assumption that the Value the salvaged DIExpression operates on is the first operand of the original instruction -- which the original code appears to assume anyway.

There is likely a small performance downside of this, as the original implementation performed the outer (i.e., instruction-type) tests only once then iterated over a collection of debug users, wheras now the outer tests occur for each debug user. While this sucks, it's a legitimate (IMHO, YMMV) trade-off for generality, and will likely be inlined away anyway. I looked at ways of preserving the previous implementation with additional abstractions, but IMHO after two layers of callbacks/lambdas it isn't worth it.

The patch display is a bit ugly in Phab but I don't think I can do much about that. Note that there's a comment on handling GEPs on origina-line 1646 that I've deleted, as it does not appear to be true.

Patch is based on D56788.

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Mon, Feb 4, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Feb 4, 9:26 AM
aprantl accepted this revision.Mon, Feb 4, 9:56 AM

This looks fine.

This revision is now accepted and ready to land.Mon, Feb 4, 9:56 AM
This revision was automatically updated to reflect the committed changes.
jmorse added a comment.Tue, Feb 5, 3:15 AM

NB, I folded a tiny NFC bit of D56788 that was necessary into the closing commit here -- the remaining discussion in the other review doesn't relate to it, and it's a no-op anyway. (Figured I'd record this for history).

Still fine.