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

Thu Mar 7 16:08:37 PST 2019

bjope added a comment.

In D58238#1421352 <https://reviews.llvm.org/D58238#1421352>, @jmorse wrote:

> I produced another patch (D59027 <https://reviews.llvm.org/D59027>, work-in-progress) that only creates undef DBG_VALUEs when the order of assignments would change... however I then realised I'd misunderstood Bjorns question here:
>
> In D58238#1406919 <https://reviews.llvm.org/D58238#1406919>, @bjope wrote:
>
> > 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.
>
>
> If I understand you correctly, this actually isn't a case that MachineSink deals with: MachineSink sinks insts from parent to child basic blocks, to remove partially redundant computations. Take this example [0], where the load of %ab is only used down one path, MachineSink moves it to only execute in the block which actually uses the load. With that in mind it might be clearer why undefs become necessary: if the DBG_VALUE moves to a different block, one of the source-level assignments to the corresponding variable has disappeared from other paths through the program. Those paths should have the variable appear ``optimised out'' until the variable gets a new location, to stop the old location being used on the other paths which would present a stale variable value (effectively this re-orders the appearance of assignments).
>
> Using the example quoted above, if ADD were sunk into a _successor_ block, then an undef DBG_VALUE would have to go somewhere, to terminate the earlier location of "x" in the other successor blocks. (NB, MachineSink doesn't operate if there's only one successor).
>
> [0] https://github.com/llvm-mirror/llvm/blob/master/test/CodeGen/X86/MachineSink-DbgValue.ll

Yes, I was just thinking out loud about sinking/scheduling in general and not so much the specific situation with MachineSink. But I also wondered if the sinking done by MachineSink could be seen as a special case of sinking in general, where we sink one instruction at a time. For MachineSink we eventually reach the end of the BB, and then continue into a successor. So how do we determine when it is time to insert "undef" when sinking one instruction at a time? What kind of reorderings should/shouldn't trigger that we insert an "undef"? I guess this is one thing we should try to describe in the documentation (also for DGB_VALUE), to make sure new developers understand the basic logic behind how we implement these things.

I only had a quick look at D59027 <https://reviews.llvm.org/D59027>, but it feels like it tries to handle this a little bit more "carefully" (avoiding <optimized out> in some situations compared to this patch). 
Btw, I'll be OOO for a week. So I won't be able to give much more feedback right now. No need to wait for my approval if you get LGTM from someone else while I'm away.

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

https://reviews.llvm.org/D58238