Page MenuHomePhabricator

[DebugInstrRef][4/9] Support recording of instruction reference substitutions
ClosedPublic

Authored by jmorse on Aug 11 2020, 9:32 AM.

Details

Summary

At various points in the post-isel optimisation passes, instructions get rewritten and replaced. One way to handle this is just to transfer the instruction number from the old to the new instruction -- however that doesn't account for passes like TwoAddressInstruction, which change the positions of operands. Instead:

  • We never re-use instruction numbers: when a new instruction is created, it gets a new number,
  • This patch adds a table of "substitutions": a mapping from old <inst,operand> pairs to the new ones.

This stems from patch 1 in this series: there's no connection between instruction numbers and DBG_INSTR_REFs preserved.

The downside of this is that we end up preserving in memory a mapping table, which more or less contains the history of optimisations that have happened to the function; and we have to apply the substitutions later, when LiveDebugValues runs. IMO this is a worthy price to pay, one table isn't highly expensive, and it models the way I'd like variable location tracking to work, doing as little work as possible during compilation and resolving variables back to locations at the end.

An additional benefit of changing instruction numbers when the instruction is recreated / modified is that changes which destroy the creator of a value can be expressed, by not creating a substitution for the old <inst,operand> pair. An example would be, if there were a divide instruction that generated the quotient and remainder, and it were replaced by one that only generated the quotient:

$rax, $rcx = div-and-remainder $rdx, $rsi, debug-instr-num 1
DBG_INSTR_REF 1, 0
DBG_INSTR_REF 1, 1

Would become

$rax = div $rdx, $rsi, debug-instr-num 2
DBG_INSTR_REF 1, 0
DBG_INSTR_REF 1, 1

With a substitution entered from <1, 0> to <2, 0>, and no substitution created for <1, 1> as it's no longer generated.

This patch only adds the data structure and MIR format, plus round-trip test.

Diff Detail

Event Timeline

jmorse created this revision.Aug 11 2020, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 9:32 AM
aprantl accepted this revision.Aug 11 2020, 11:37 AM
This revision is now accepted and ready to land.Aug 11 2020, 11:37 AM
This revision was landed with ongoing or failed builds.Thu, Oct 15, 3:31 AM
This revision was automatically updated to reflect the committed changes.
jmorse reopened this revision.Thu, Oct 15, 5:37 AM

After landing this, bkramer pointed out one of the assertions in substituteDebugValuesForInst was ineffective. Fixing that shows that some callers (see patch 5 in this series) do so with different instruction signatures (i.e., different types and different numbers of operands).

I've revised the method in the patch I'll upload in a few seconds to optionally allow a "number of operands to substitute" to be specified. This makes use of the LLVM convention that register defs are usually the first operands: we can say for the arithmetic-to-LEA code paths (patch 5) that only the first operand needs to be substituted.

I don't think this changes the fundamental idea of this patch, only the way it's expressed. I'll leave this up for a few days and then land.

This revision is now accepted and ready to land.Thu, Oct 15, 5:37 AM
jmorse updated this revision to Diff 298355.Thu, Oct 15, 5:38 AM

Updated patch, changes signature of substituteDebugValuesForInst to take a "number of operands to look at" argument, defaults to "all of them".