[DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink

[mattd]

Summary

The logic for handling the sinking of COPY instructions was generating
different code when building with debug flags.

The original code did not take into consideration debug instructions. This
resulted in the registers in the DBG_VALUE instructions being treated as used,
and prevented the COPY from being sunk. This patch avoids analyzing debug
instructions when trying to sink COPY instructions.

This patch also creates a routine from the code in MachineSinking::SinkInstruction to
perform the logic of sinking an instruction along with its debug instructions.
This functionality is used in multiple places, including the code for sinking COPY instrs.

[vsk]

Can this introduce a use-before-def scenario, where the DBG_VALUE for $eax occurs before the copy?

[bjope]

In D45637#1067785, @vsk wrote:

Can this introduce a use-before-def scenario, where the DBG_VALUE for $eax occurs before the copy?

I think it is the same kind of problem with sinking as I'm trying to solve (for instcombine) here: https://reviews.llvm.org/D45425
I suppose a similar solution would be OK, even if we are beyond SSA form here. So we should sink any DBG_VALUE with a debug-use of the register being defined by the sunken COPY.

[mattd]

In D45637#1067916, @bjope wrote:

In D45637#1067785, @vsk wrote:

Can this introduce a use-before-def scenario, where the DBG_VALUE for $eax occurs before the copy?

I think it is the same kind of problem with sinking as I'm trying to solve (for instcombine) here: https://reviews.llvm.org/D45425
I suppose a similar solution would be OK, even if we are beyond SSA form here. So we should sink any DBG_VALUE with a debug-use of the register being defined by the sunken COPY.

I think you both are correct, thanks for the feedback!

[mattd]

• Create the performSink routine from code already used to sink an instruction and its associated debug information.
• Use this routine to also sink debug information associated to COPY instructions.
• Update the test to check that the DBG_VALUE instruction moves/sinks.

[vsk]

This looks good to me. It'd be great to have a +1 from someone who's contributed to this pass.

[mattd]

I'm curious, is this issue on anyone else's radar? I know @jonpa was working on solving this, including a handful of other similar cases. If this patch isn't needed anymore then I'm happy to abandon it. If we do want it, then I am happy to rebase, since it's a bit stale now :)

[vsk]

I'd like to see this move forward. I'll be out of the office over the next week. I can try to help find another reviewer when I'm back.

[bjope]

This comment in the description does not make sense to me:

The original logic intentionally ignores all call instructions. Since debug intrinsics
are represented as calls, the pass was skipping that block and not
performing the sink.

The debug intrinsic is no longer a call after being lowered to DBG_VALUE. So it must have been something else that caused the debug invariant behavior, right?
I guess it was the call to TII->trackRegDefsUses (on trunk it is LiveRegUnits::accumulateUsedDefed), done for non COPY instructions, that would track register uses in debug instructions unless we avoid that somehow.

Could you update the description to make it less confusing?

lib/CodeGen/MachineSink.cpp

1037

Nowadays we have isDebugInstr() to cover all kinds of debug instructions. I suggest using that instead here, to make the check more general.

[mattd]

Thanks for the review @bjope!

My apologies, you are correct, the registers in the DBG_VALUE were getting
captured in LiveRegUnits::accumulateUsedDefed I fixed up the summary
and a few comments in the code/test as well.

• Updated the patch to use isDebugInstr().
• Cor rected some comments and renamed the test to reflect that the functionality is trying to avoid processing debug instructions.

[bjope]

Basically looks good to me now, as I think that getting rid of the debug-invariant problem is important.
When it comes to the sinking of DBG_VALUE instructions I believe that will be as good as the pre-RA version of MachineSinking. I think that could be enough to land this (as getting rid of the debug-invariance probably is more important).

It is probably easy, at least by hand, to write some MIR-tests that show that the sinking of DBG_VALUE instruction could be more complicated in general, compared to what performSink currently can handle.
I'm thinking about something like this:

...
renamable $eax = COPY $edi
DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16
$esi = ADD $edi, 7
DBG_VALUE debug-use $esi, debug-use $noreg, !14, !DIExpression(), debug-location !17
...

In the above code code the variable !14 is given a new value after the ADD. Afaict the performSink method (or actually collectDebugValues) simply decides to sink all DBG_VALUE instructions that is using $eax when sinking the COPY defining $eax. But if we sink the first DBG_VALUE past the second DBG_VALUE it will look as if the variable will get the old value when single-stepping past the sunken COPY. I'm not sure, but I don't think that it should be like that.

I think that performSink should look at other DBG_VALUE instructions to see if they refer to the same variable, and if so probably drop the DBG_VALUE (unless it can be salvaged in some way) instead of sinking it. Although I'm not sure how common code patterns like the one above are in reality. Maybe we can improve performSink in a later patch.

[mattd]

@bjope , Thanks for your review and example! I agree that any changes to improve performSink/collectDebugValues should probably be in another patch.

I agree, after spending some time with your hypothetical case, I can imagine there are some other cases that might result in old/intermediate values in the user's variable, at least for a short period of time.

In D45637#1129560, @bjope wrote:

...
renamable $eax = COPY $edi
DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16
$esi = ADD $edi, 7
DBG_VALUE debug-use $esi, debug-use $noreg, !14, !DIExpression(), debug-location !17
...

Excuse me if I am getting this wrong, the following is my interpretation of your example. Let's assume this example lives in bb0. In the example, sticking with x86_64, the ADD (or ADD32ri) instruction expects tied physical registers, something like $edi = ADD32ri $edi, 7 However, that case would prevent the COPY from sinking, I'm guessing due to a register def. If we were to make this $esi = ADD32ri $esi, 7, that would allow for the COPY to be sunk. Variable !14 would truly be $esi +7 at that program point in bb0, because the DBG_VALUE you introduced will associate $esi as the value for !14. The sunken copy, presumably in bb1, would be the value copied from $edi, and probably the value that a user would expect. Single stepping from the ADD to the sunken COPY would show !14 as being an intermediate or 'old' value until the sunken copy is reached. This doesn't seem wrong to me. It just could present unexpected values to the user, when inspecting !14 between the ADD and sunken COPY. Perhaps, if we were to delete just the DBG_VALUE associated to the ADD in bb0 , perhaps that would encourage a debugger to present 'optimized-out' in this case. I agree with you, any refinements to performSink or collectDebugValues should probably be for another patch. Thanks again for your review.

[bjope]

In D45637#1130313, @mattd wrote:

@bjope , Thanks for your review and example! I agree that any changes to improve performSink/collectDebugValues should probably be in another patch.

I agree, after spending some time with your hypothetical case, I can imagine there are some other cases that might result in old/intermediate values in the user's variable, at least for a short period of time.

In D45637#1129560, @bjope wrote:

...
renamable $eax = COPY $edi
DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16
$esi = ADD $edi, 7
DBG_VALUE debug-use $esi, debug-use $noreg, !14, !DIExpression(), debug-location !17
...

Excuse me if I am getting this wrong, the following is my interpretation of your example. Let's assume this example lives in bb0. In the example, sticking with x86_64, the ADD (or ADD32ri) instruction expects tied physical registers, something like $edi = ADD32ri $edi, 7 However, that case would prevent the COPY from sinking, I'm guessing due to a register def. If we were to make this $esi = ADD32ri $esi, 7, that would allow for the COPY to be sunk. Variable !14 would truly be $esi +7 at that program point in bb0, because the DBG_VALUE you introduced will associate $esi as the value for !14. The sunken copy, presumably in bb1, would be the value copied from $edi, and probably the value that a user would expect. Single stepping from the ADD to the sunken COPY would show !14 as being an intermediate or 'old' value until the sunken copy is reached. This doesn't seem wrong to me. It just could present unexpected values to the user, when inspecting !14 between the ADD and sunken COPY. Perhaps, if we were to delete just the DBG_VALUE associated to the ADD in bb0 , perhaps that would encourage a debugger to present 'optimized-out' in this case. I agree with you, any refinements to performSink or collectDebugValues should probably be for another patch. Thanks again for your review.

I think you got it (even though I see the value from "debug location !16" as the old value, so by reordering the DBG_VALUE statements it will look like the variable first gets the value from "debug location !17" and then the value from "debug location !16", which could be the opposite from what is expressed in the source code that we debug).

Btw, the "ADD" was just an example. We can even skip the "ADD" and make the DBG_VALUE refer to a constant value:

  ...
bb.0:
  $edi = "load variabel !14"
  renamable $eax = COPY $edi
  DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16
  <some code allowing the COPY to be sunk past this>
  DBG_VALUE i32 555, debug-use $noreg, !14, !DIExpression(), debug-location !17
  ...
bb.1:
  ....

let's say this origins from C code like this

int var14 = X;           // Some stack allocated variable
...
int tmp = Var14;     // location !16
...
Var14 = 555;           // location !17
if (<some cond>) {
  bar = tmp;
  <code block A>
  return;
}
...

My concern was more about what value var14 (that is !14) will appear as having in <code block A>. If we reorder the DBG_VALUE instructions, then !14 might appear as having the old value X in <code block A> (even if we have stepped past both location !16 and !17).

[mattd]

That's a great explanation, thank you!

[mattd]

Just pinging this. @bjope has provided great feedback (thanks!), but I am wondering if anyone else thinks this patch is useful, or if I should abandon it in favor of some other solution?

[bjope]

In D45637#1137912, @mattd wrote:

Just pinging this. @bjope has provided great feedback (thanks!), but I am wondering if anyone else thinks this patch is useful, or if I should abandon it in favor of some other solution?

FWIW: I just want to clarify that I think this patch is better than doing nothing at all. And I'm not sure exactly what should be expected when it comes to reordering DBG_VALUE instructions referencing the same variable. After all, if we do heavy optimizations the user must be aware that debug-experience can be confusing sometimes, so maybe I was overthinking the situation.

@aprantl or @probinson usually have a good feeling about what is reasonable to do. Have you seen this discussion?

[aprantl]

That's an interesting question. I could see both sides of the argument:

• if the code has been reordered such that line 1 is executed after line 2, then the debug info should reflect that
• the optimizer should never move a debug intrinsic over another intrinsic

I think this boils down to what the debug locations on the surrounding instructions are. If the debug location of the next instruction (or perhaps the instruction the dbg value is using as a location) is indicating that we are going back in time, then it seems okay to move the debug value. I'm afraid that this judgment needs to be done on a case-by-case basis for every transformation.

lib/CodeGen/MachineSink.cpp

764

Can you add a sentence explaining *why* merging vs. erasing is the right thing to do in the respective cases?

[probinson]

Moving DBG_VALUE instructions along with the instructions that define the associated values is the right thing to do.

So the change is, uses by debug instructions no longer affect the decision to move the COPY, and (now that they don't prevent the sink) move said debug instructions along with the COPY.
That all sounds good. I don't doubt there are more places where this needs to happen, but it seems like it can only help here.

[probinson]

In D45637#1137975, @aprantl wrote:

• the optimizer should never move a debug intrinsic over another intrinsic

Sorry, can you expand on that side of the argument?

[aprantl]

In D45637#1137982, @probinson wrote:

In D45637#1137975, @aprantl wrote:

• the optimizer should never move a debug intrinsic over another intrinsic

Sorry, can you expand on that side of the argument?

Roughly what I had in mind is a scenario like this. My example is probably a little silly:

if (p == NULL)
  b = 1;
p = q;
bar()

an in optimized LLVM IR:

call dbg.value(!DIVariable("p", ...), null, !DIExpression(DW_OP_constu 0))
%b = predicated_move(%p, i32 1) // assuming such an instruction exists :-)
%p.1 = %q
call dbg.value(!DIVariable("p", ...), %p.1,  !DIExpression())

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.

[mattd]

lib/CodeGen/MachineSink.cpp

764

I lifted this bit of the code from the original implementation and placed it into a routine to be shared by a few functions in this file (SinkInstruction and tryToSinkCopy). Mainly, I wanted tryToSinkCopy to account for the movement of any adjacent debug values associated with the to-be-sunken copy, and the code was already there for that functionality, but in SinkInstruction. We need that functionality for tryToSinkCopy too.

With that said, it seems to me that the intention here (to merge or erase), is a conservative approach.
getMergedLocation, in this form, will only accept a location if both the sunken instruction and its destination have the same (non-discriminated) location, otherwise the location is erased. I have heard word that getMergedLocation might change, but that's probably a separate discussion. I think the comment should probably be updated as:
"Only preserve MI's location if it is the same as InsertPos. This removes the location in cases
where MI is landing in a different, or empty, block from what the location is currently set to. By conservatively erasing the debug location, we also eliminate the potential for reporting wrong line information when observing the sunken instruction via debug-info driven utilities."

[probinson]

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

[mattd]

@aprantl Thanks for example.

In D45637#1138134, @probinson wrote:

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

[probinson]

As I had noted earlier, once you modify the pass so that the debug instruction doesn't interfere with the code motion, the associated debug instructions have to move with the real instruction. In that respect, the patch is doing the right thing.

I am unconvinced that it is a real, practical problem to treat real instructions and their associated debug info as a unit. Once we have a demonstrable problem, we can work out how to deal with it.

[aprantl]

I have no problem with option (3).

[bjope]

We all have some suspicions that it might be awkward to move a DBG_VALUE intrinsic across another one for the *same* *variable*. But we haven't seen that happen in this particular situation when having an end-to-end test with C/C++ input. I'm pretty sure such things can happen in other passes (e.g. CodegenPrepare is moving dbg.value without any checks at all), so it is more like a general concern.

And it seems like we agree that alternative 3 ("Land this patch, if there is nothing obviously wrong :), and then file a bug for the improvement noted above.") is the way forward.

LGTM!

[mattd]

Thanks again for all of the input on this issue.

[mattd]

I have filed PR37897 to keep track of the issue mentioned above: The sinking, or more generally moving, of dbg values across other dbg values.
https://bugs.llvm.org/show_bug.cgi?id=37897

lib/CodeGen/MachineSink.cpp

 /// SinkInstruction - Determine whether it is safe to sink the specified machine
 /// instruction out of its current block into a successor.
 bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
                                      AllSuccsCache &AllSuccessors) {
   // Don't sink instructions that the target prefers not to sink.
   if (!TII->shouldSink(MI))
     return false;
 
   // Check if it's safe to move the instruction.
   if (!MI.isSafeToMove(AA, SawStore))
     return false;
 
   // Convergent operations may not be made control-dependent on additional
   // values.
   if (MI.isConvergent())
     return false;
 
bool PostRAMachineSinking::tryToSinkCopy(MachineBasicBlock &CurBB,
   // Track which registers have been modified and used between the end of the
   // block and the current instruction.
   ModifiedRegs.reset();
   UsedRegs.reset();
 
   for (auto I = CurBB.rbegin(), E = CurBB.rend(); I != E;) {
     MachineInstr *MI = &*I;
     ++I;
+    if (MI->isDebugValue())
+      continue;
 
     // Do not move any instruction across function call.
     if (MI->isCall())
       return false;
 
     if (!MI->isCopy() || !MI->getOperand(0).isRenamable()) {
       TII->trackRegDefsUses(*MI, ModifiedRegs, UsedRegs, TRI);
       continue;

test/CodeGen/X86/postra-ignore-dbg-intrinsics.mir

+# RUN: llc -mtriple=x86_64-none-linux-gnu -run-pass=postra-machine-sink -verify-machineinstrs  -o - %s | FileCheck %s
+#
+# This test was originally generated from the following sample:
+#
+# int x0;
+# extern void x3(int, int);
+# void x1(int x2) {
+#   if (x0)
+#     x3(0, x2);
+# }
+#
+# The code generates a COPY instruction which the PostRA Machine Sink pass will
+# try to sink.  Earlier versions were not performing the sink due to a
+# debug intrinsic confusing the sinking algorithm.
+
+--- |
+  @x0 = common dso_local global i32 0, align 4, !dbg !0
+
+  define dso_local void @x1(i32) !dbg !11 {
+    %2 = alloca i32, align 4
+    store i32 %0, i32* %2, align 4
+    call void @llvm.dbg.declare(metadata i32* %2, metadata !14, metadata !DIExpression()), !dbg !16
+    %3 = load i32, i32* @x0, align 4, !dbg !16
+    %4 = icmp ne i32 %3, 0, !dbg !16
+    br i1 %4, label %5, label %7, !dbg !16
+
+  ; <label>:5:                                      ; preds = %1
+    %6 = load i32, i32* %2, align 4, !dbg !16
+    call void @x3(i32 0, i32 %6), !dbg !16
+    br label %7, !dbg !16
+
+  ; <label>:7:                                      ; preds = %5, %1
+    ret void, !dbg !16
+  }
+
+  declare void @llvm.dbg.declare(metadata, metadata, metadata)
+  declare dso_local void @x3(i32, i32)
+
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!7, !8}
+
+  !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+  !1 = distinct !DIGlobalVariable(name: "x0", scope: !2, file: !3, line: 1, type: !6, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+  !3 = !DIFile(filename: "test.c", directory: "")
+  !4 = !{}
+  !5 = !{!0}
+  !6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !7 = !{i32 2, !"Dwarf Version", i32 4}
+  !8 = !{i32 2, !"Debug Info Version", i32 3}
+  !11 = distinct !DISubprogram(name: "x1", scope: !3, file: !3, line: 3, type: !12, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !2, variables: !4)
+  !12 = !DISubroutineType(types: !13)
+  !13 = !{null, !6}
+  !14 = !DILocalVariable(name: "x2", arg: 1, scope: !11, file: !3, line: 3, type: !6)
+  !15 = distinct !DILexicalBlock(scope: !11, file: !3, line: 4, column: 7)
+  !16 = !DILocation(line: 4, column: 7, scope: !15)
+
+...
+---
+# CHECK-LABEL: name: x1
+# CHECK-LABEL: bb.0:
+# CHECK-NOT: $eax = COPY $edi
+# CHECK-LABEL: bb.1:
+# CHECK: liveins: $edi
+# CHECK: renamable $eax = COPY $edi
+# CHECK-LABEL: bb.2:
+name:            x1
+alignment:       4
+tracksRegLiveness: true
+body: |
+  bb.0:
+    successors: %bb.2(0x30000000), %bb.1(0x50000000); %bb.2(37.50%), %bb.1(62.50%)
+    liveins: $edi
+    DBG_VALUE debug-use $edi, debug-use $noreg, !14, !DIExpression(), debug-location !16; line no:3
+    renamable $eax = COPY $edi
+    DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !16; line no:3
+    CMP32mi8 $rip, 1, $noreg, @x0, $noreg, 0, implicit-def $eflags, debug-location !16 :: (dereferenceable load 4 from @x0)
+    JE_1 %bb.2, implicit killed $eflags, debug-location !16
+    JMP_1 %bb.1, debug-location !16
+
+  bb.1:
+    liveins: $eax
+    $edi = MOV32r0 implicit-def dead $eflags, debug-location !16
+    $esi = COPY killed renamable $eax, debug-location !16
+
+  bb.2:
+    RET 0, debug-location !16
+...