[PATCH] D64630: [DebugInfo] Address performance regression with r364515

[Jeremy Morse via Phabricator via llvm-commits]

jmorse updated this revision to Diff 229915.
jmorse added a subscriber: andreadb.
jmorse added a comment.

Hokay, here's another take on this problem. To recap, all my previous implementations were broken because of

  a) performance problems looking up slot indexes for DBG_VALUEs, and
  b) I wasn't aware that the coalescer will merge overlapping live ranges.

which I'll address in order below.

This implementation is inspired by Vedants sketch -- we build a data structure for looking up DBG_VALUEs by slot index quickly. For this (bear with me) I've mapped VRegs -> a set of {SlotIndex,MachineInstr*} pairs. In the body of checkMergingChangesDbgValuesImpl, we can then use the set-order to simultaneously advance through:

- The live ranges of the VReg being merged, and
- The set of all DBG_VALUEs for the other VReg.

to identify those DBG_VALUEs at risk of unsound merging. This avoids having to perform a slot index lookup at all, at the cost of stepping through valid DBG_VALUEs in the process. For an asan build of clang-3.4 I don't observe any change in build time with this change. For the previous worst-case file, AMDGPUDisassembler.cpp on trunk/master, the register coalescing pass increases from 2.9 seconds to roughly 3.5 seconds (out of an overall compile time of 27 seconds). This is, IMO, quite good given that it's dealing with pathological conditions (the packs of 800 DBG_VALUEs in a row).

For the correctness issue, observe the ShouldUndef lambda in the patch. This identifies when overlapping ranges have been merged, and examines RegisterCoalescers record of how it resolved the conflict (details in the comment). This probably wants attention from people who know the register allocator/coalescer -- the aim is to ensure DBG_VALUEs, which don't contribute to liveness, do not refer to a different live value-number after coalescing. We don't care if they refer to a non-live vreg. (Paging @andreadb , this is what I was going to mention, a uh, while ago).

Running through llvm-dwarfdump --statistics, the same as before, a tiny fraction of variables go missing (<0.02%). This is fine IMO, as we're dropping locations that are broken.

Rough edges:

- It's not great to make JoinVals and RegisterCoalescer friends; I'll introduce an accessor for examining conflict resolutions later, but I've burnt out of time today.
- I haven't made an attempt to address vregs being merged with physregs in this patch -- if this seems to be going in the right direction, I'll extend it to that.


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

https://reviews.llvm.org/D64630

Files:
  llvm/lib/CodeGen/RegisterCoalescer.cpp
  llvm/test/DebugInfo/MIR/X86/regcoalescing-clears-dead-dbgvals.mir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64630.229915.patch
Type: text/x-patch
Size: 15059 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191118/f96bb719/attachment-0001.bin>