[PATCH] D58238: [DebugInfo] MachineSink: Insert undef DBG_VALUEs when sinking instructions, try to forward copies

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 4 inline comments as done.
jmorse added inline comments.


================
Comment at: lib/CodeGen/MachineSink.cpp:798
+        // copy. Only forward the copy if the DBG_VALUE operand exactly
+        // matches the copy destination.
+        if (DbgMO.getReg() == DstMO.getReg())
----------------
aprantl wrote:
> I may be misunderstanding, but if it isn't a vreg, don't we need to check that there are no defs of the reg in between? Or is this all pre-ra?
Good question -- this code is called by both pre and post RA code. For post-ra it's guaranteed that there are no defs in between by the calling code: otherwise it would not be legal to sink the copy in the first place. For pre-ra we can rely on the SSA-ness of the function to guarantee validity.

However, I've been using the "both-are-vregs" and "both-are-physregs" tests as a proxy for whether we're pre or post RA, which isn't necessarily sound. I don't believe the pre-ra machine sinker will sink anything with meaningful physreg operations, but I've added an explicit test to make this more robust.


================
Comment at: lib/CodeGen/MachineSink.cpp:807
+      } else {
+        DbgMI->getOperand(0).setReg(0);
+      }
----------------
aprantl wrote:
> Does this get easier to read if you unconditionally call setReg(0) and then overwrite it in the copy case?
It does, I've done that and simplified the tests into two (large) conditionals.


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

https://reviews.llvm.org/D58238