Page MenuHomePhabricator

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

Authored by jmorse on Feb 4 2019, 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

Event Timeline

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

This looks fine.

This revision is now accepted and ready to land.Feb 4 2019, 9:56 AM
This revision was automatically updated to reflect the committed changes.
jmorse added a comment.Feb 5 2019, 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.