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

Fri Feb 22 05:44:03 PST 2019

jmorse planned changes to this revision.
jmorse marked an inline comment as done.
jmorse added a comment.

In D58238#1406919 <https://reviews.llvm.org/D58238#1406919>, @bjope wrote:

> So then we should check if there is any other dbg.value that we sink past? Otherwise there won't be any reordering and no need for an undef location?

True -- I'd previously been ignoring this issue as I thought it'd require another re-scan of instructions we sink past, but as the calling code walks through blocks backwards, it should be easy to maintain a little extra state recording what's been seen. (I might roll this into the parent patch too).

> When for example regalloc inserts spill/reload code, I guess that it in some sense can be seen as the spill is hoisted from the reload, so in effect it sinks lots of instructions past the spilling instruction. But it does not seem correct to sprinkle undef DBG_VALUE instructions all over the code, in such situations.

I don't completely follow this, a vreg that gets spilt-and-reloaded has a location even when it's spilt surely? (How livedebugvalues and debug locations for spilt values works isn't something I've looked into).

> Anyway, I'm still a little bit confused, and haven't really understood the full consequence of this.
>  Just got a feeling that some criterias for when to do this is missing, but maybe the aptch just takes a defensive appoach. We do not really need to insert the undef location always, but we need to do it sometimes. So it is better to do it too often compared to too seldom (as having an undef location always is OK).

It's definitely a defensive approach, as covered above I tend to prioritise not allowing unsound variable locations, even when we could be more complete.

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

https://reviews.llvm.org/D58238