[PATCH] D70676: [DebugInfo] Don't repeatedly created undef DBG_VALUEs during machine-sinking

[Jeremy Morse via Phabricator via llvm-commits]

jmorse reopened this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

Too lazy on the earlier explanation there sorry. To elaborate: I thought I'd accounted for nondeterminism in the two loops in reinsertSunkDebugDefs, but both were a bit broken:

For the outer loop, because we're only ever placing DBG_VALUEs after pre-determined instructions like this:

  inst1
  <--- insert DBG_VALUE here
  inst2
  <--- insert DBG_VALUE here

I'd assumed that it didn't matter what order that happens in. However, on second thoughts if an instruction defines more than one vreg, groups of DBG_VALUEs associated with different vregs from an instruction could change order.

For the inner loop, I was moving elements into a vector and then sorting them, based on the "DebugVariable" classes `operator<`. However, it turns out that compares pointers, and more generally it seems metadata doesn't support being ordered.

For both of these problems I've now switched to MapVector and SetVector respectively; the insertion order is defined by MachineSinking iterating over basic blocks (which is stable AFAIUI), and MachineSinking::ProcessBlock walking through the block backwards, which is also stable.

In the output from hans' reproducer, packs of DBG_VALUEs were being re-ordered, but in a way that didn't change the meaning of any variable location. I don't know why that led to changes in the layout of location lists (again, the meaning was the same), but either way the re-ordering is bad.


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

https://reviews.llvm.org/D70676