[PATCH] D106856: [LiveDebugValues] Cleanup Transfers when removing Entry Value Location

[Nikola Tesic via Phabricator via llvm-commits]

ntesic added a comment.

Hi Jeremy,
Thanks for the comments.

This patch purpose is to fix PR47628 <https://bugs.llvm.org/show_bug.cgi?id=47628> and improve
the solution from D68209 <https://reviews.llvm.org/D68209>  .
Here is the more detailed explanation.

Typical MIR in focus of the patch:

  $edi = MOV
  DBG_VALUE $edi, ,"param"

Step_1: MI: $edi = MOV ...

- LDV detects parameter forwarding register is clobbered.
- As the primary location of parameter is lost, we use its EntryValue Backup location as the new primary location, and emit the Transfer.

Step_2: MI: DBG_VALUE $edi, ,"param"

- LDV detects parameter value is modified.
- The EntryValue Backup location of the parameter is not valid anymore.

Problem:
Step_2 means we shouldn't have used EntryValue Backup location in Step_1.

Old solution:

- removes EntryVarLoc from EntryValue Backup locations
- not from primary locations (OpenRanges) if inserted
- doesn't invalidate already emitted Transfer

Solution from this patch - do the cleanup to undo the Step_1:

- remove emitted EntryValueTransfer
- remove EntryVarLoc from OpenRanges

Bad MIR example from PR47628:

      renamable $edi = MOV32rm $rip, 1, $noreg, @value, $noreg, debug-location !20
  [1] DBG_VALUE $edi, $noreg, !13, !DIExpression(DW_OP_LLVM_entry_value, 1), [...]
      DBG_VALUE $edi, $noreg, !13, !DIExpression(), debug-location !14
      ...
      CALL64pcrel32 @bar, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit-def $rsp, implicit-def $ssp, debug-location !20
  [2] DBG_VALUE $edi, $noreg, !13, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location !13

Consequences of the main problem:
[1] Not-deleted Transfer that uses parameter Entry Value will result in bad DBG_VALUE in MIR.
[2] Not-deleted Entry Value Location from Open Ranges will result in usage of incorrect VarLoc later in code.

> As I understand it, this patch is aiming to recognise patterns where an entry value is being copied to a new register location and then being DBG_VALUE'd, which is effectively a no-op as far as tracking entry values is concerned.

This patch is not focused on copyies of entry values. As I understand, that is already included in D68209 <https://reviews.llvm.org/D68209>  .

> I think the EntryValTransfers container is going to suffer from not being aware that it's part of a dataflow algorithm: one transfer gets inserted per emitEntryValues, and we can repeatedly explore the same block in a loop as part of the dataflow algorithm. But cleanupEntryValueTransfers will only delete one entry -- I suspect it should try and delete all matching entries. (The uh, Transfers collection suffers from the same problem, which I had a patch for at D67500 <https://reviews.llvm.org/D67500>, but didn't push it particularly hard).

The EntryValTransfers container is managed in the similar way as the Transfers.
But, as it is used to do cleanup only for the previous register defining instruction, I don't see that as the problem further in dataflow algorithm.
(In short terms, if we find DBG_VALUE of changed parameter, do the cleanup only for the instruction that created that DBG_VALUE.)

> The RegSetterInstructionMap is purely to find COPYs that might mean the entry value doesn't have to be dropped, yes? This seems OK, so long as it's not relied upon to determine whether a register contains a different value -- values can be defined when blocks merge and the join method runs.

When we find new DBG_VALUE of a parameter, if its debug operand is a Register, find which instruction clobbered the Register using RegSetterInstructionMap.
Then we can map that instruction to an emitted Entry Value Transfer.
The insertion into RegSetterInstructionMap is tied to definitions of a registers found by the transferRegisterDef function, so we don't pay attention to the value loaded into the Register.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106856/new/

https://reviews.llvm.org/D106856