[PATCH] D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

@aprantl  Thanks for example.

In https://reviews.llvm.org/D45637#1138134, @probinson wrote:

> In https://reviews.llvm.org/D45637#1138063, @aprantl wrote:
>
> > If we were to reorder the instruction stream so that the two intrinsics are swapped, it will look like the value of p is NULL throughout the rest of the program.
>
>
> Ah!  We don't want to move a value intrinsic across another one for the *same* *variable* hmmm.
>  That feels like one of the stores would be dead?  I'm not sure what we'd want the debug info to look like in that case.


That makes sense, and after fiddling around with my .mir case.  It looks like I can get into a similar situation, of course that is hand modified .mir code,
and it doesn't affect correctness, rather just might result in a confused debug representation for the user.

It seems like fixing that case (or improving how we move debug values across other debug values for the same variable) would be more involved than addressing what this patch is trying to solve.

I see a few options moving forward:

1. Try to come up with a case where user source code (not .mir) would generate such a situation (and we have some good starting examples).  And if we can recreate the confused debug presentation, then fix that as part of this patch.

2. Split this patch off into two separate patches, as I see two separate ideas/fixes here.

The first patch being the introduction of the `if (MI->isDebugInstr()) continue;` in `tryToSinkCopy`.  That would at least
preserve the object code representation between optimized vs debug-enabled compilations.  Likewise, that would
also permit optimization in the case of debugging.

The second patch would then address the representation of the debug instructions, as I think the consistency problem isn't isolated to just the sinking of COPY, but probably also the sinking of any other instruction.
Of course, I can also see the argument where if we do open the doors for tryToSinkCopy to optimize debuggable code, the debug representation should be correct.

3. Land this patch, if there is nothing obviously wrong :), and then file a bug for the improvement noted above.


https://reviews.llvm.org/D45637