[PATCH] D53992: [DebugInfo] Correctly sink DBG_VALUEs in postra-machine-sink

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 9 inline comments as done.
jmorse added a comment.

Hi,

In https://reviews.llvm.org/D53992#1284437, @probinson wrote:

> Presumably this addresses the bad debugging experience for the sample from the PR; does it have any other good/bad effect on the Dexter corpus?


Alas there's no other effect on the rest of the tests, only the original [0], although we also get the correct lifetime for the function argument now. (Previously it was "optimized out" everywhere after the start of the function).

[0] https://github.com/jmorse/dexter/blob/dextests/tests/nostdlib/llvm_passes/Vectorize/MissingArgVal/missingargval.cpp



================
Comment at: lib/CodeGen/MachineSink.cpp:960
+  /// Track DBG_VALUEs of (unmodified) register units
+  std::multimap<unsigned, MachineInstr *> SeenDbgInstrs;
+
----------------
rnk wrote:
> aprantl wrote:
> > Would a (Small)DenseMap<SmallVector> be more efficient?
> Peanut gallery: DenseMap of Small* things are usually not efficient. There is TinyPtrVector, though, which is optimized for this use case, so `DenseMap<unsigned, TinyPtrVector<MachineInstr*>>` is probably a good choice.
Now using the suggested container; running a Debug clang/llvm build again gave one faster time, one slower time, both within the margin of error. It looks like this container form will match the common case very well, and in the worst case be no worse than multimap.


================
Comment at: lib/CodeGen/MachineSink.cpp:1130
+    // it may not be anywhere near the register def
+    if (MI->isDebugValue()) {
+      auto &MO = MI->getOperand(0);
----------------
mattd wrote:
> Can you get away with using MI->isDebugInstr ?
I don't believe so -- that matches DebugLabels as well which don't get sunk, so there'd need to be another condition filtering them out.

(NB, at this time I don't really know what DebugLabels are used for, so this opinion isn't based on any knowledge).


================
Comment at: lib/CodeGen/MachineSink.cpp:1140
+        // Record debug use of this register
+        SeenDbgInstrs.insert(std::make_pair(MO.getReg(), MI));
+      }
----------------
mattd wrote:
> As an alternative to insert, you can call emplace().  
> SeenDbgInstrs.emplace(MO.getReg(), MI);
(Container changed to one that doesn't have emplace).


https://reviews.llvm.org/D53992