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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

>> Maybe it is a philosophical question. Is the compiler scheduling/reordering source instructions (in the debugger it will appear as source statements are executed in a random order), or are we scheduling/reordering machine instructions (and in the debugger it still should appear as if we execute variable assignments in the order of the source code). VLIW-scheduling, tail merging, etc, of course make things extra complicated, since we basically will be all over the place all the time.
> 
> To me it's the latter, my mental model (maybe wrong) is that while the compiler is optimising code, for debuginfo it has a state-machine of variable locations represented by dbg.values that it needs to try and preserve the order of, marking locations undef if they don't exist any more. The logical consequence is that if we had a function where the compiler managed to completely reverse the order of computation, no variable would have a location, which would be sound, but not useful.

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?
It might be that we sink past instructions that has been sunk earlier (or simply been re-scheduled at ISel etc), so it could be that we restore the source order when sinking.  Or we could be sinking past some constant materialisation that has no associated source line. Etc.

After ISel we could have something like this:

  %1 = LOAD ...
  DBG_VALUE %1, %noreg, "x", ...
  %2 = AND ...
  %3 = ADD %1, 3
  DBG_VALUE %3, %noreg, "x", ...
  %5 = SUB 0, 0

or we could just as well have the SUB before the ADD afaict

  %1 = LOAD ...
  DBG_VALUE %1, %noreg, "x", ...
  %2 = AND ...
  %5 = SUB 0, 0
  %3 = ADD %1, 3
  DBG_VALUE %3, %noreg, "x", ...

Sinking the ADD past the SUB would introduce an undef location for "x", when done by MachineSink. But not if the instructions were emitted in that order already at ISel.
So minor changes in ISel-scheduling could ripple down, impacting how many undef DBG_VALUE we see after MachineSink. It is not like ISel care that much about where it inserts this SUB afaict.

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.

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).


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

https://reviews.llvm.org/D58238