[CodeGen] Skip over dbg-instr in twoaddr pass

[markus]

Summary

A DBG_VALUE between a two-address instruction and a following COPY would prevent rescheduleMIBelowKill optimization inside TwoAddressInstructionPass.

[aprantl]

lib/CodeGen/TwoAddressInstructionPass.cpp

932 ↗

(On Diff #179246)

For IR we have an iterator that skips over debug intrinsics automatically. Does the same thing exist for MIR, too?

[vsk]

Thanks for the patch!

lib/CodeGen/TwoAddressInstructionPass.cpp

932 ↗

(On Diff #179246)

Yes, I believe we have skipDebugInstructionsForward / skipDebugInstructionsBackward. It might be neat to use that here.

test/CodeGen/X86/twoaddr-dbg-value.mir

12 ↗

(On Diff #179246)

This is mostly an academic question, but: is this a valid DBG_VALUE? I'm a little surprised this parses (I'd expect the instruction to have more operands), but I like the simplicity of it.

[markus]

lib/CodeGen/TwoAddressInstructionPass.cpp

932 ↗

(On Diff #179246)

If we were to use skipDebugInstructionsForward then it seems that the loop would have to be written as follows to preserve the semantics (and handle multiple COPYs with DBG_VALUEs interspersed in various ways).

while (End != MBB->end()) {
  End = skipDebugInstructionsForward(End, MBB->end())
  if (End->isCopy() && regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI)))
    Defs.push_back(End->getOperand(0).getReg());
  else
    break;
  ++End;
}

Would that be better?

test/CodeGen/X86/twoaddr-dbg-value.mir

12 ↗

(On Diff #179246)

It is probably not valid but does seem to be sufficient for the purpose of this test (where the presence of a DBG_VALUE is the only relevant thing and not the contents of it). I would be happy to make it more valid though if that is just a matter of adding the additional operands.

[vsk]

lib/CodeGen/TwoAddressInstructionPass.cpp

932 ↗

(On Diff #179246)

I personally find this easier to follow. Imho it's nice that the isDebugInstr check doesn't need to be duplicated.

test/CodeGen/X86/twoaddr-dbg-value.mir

12 ↗

(On Diff #179246)

Ah, I see. I think this is fine the way it is. It'd be a bit messy to add the debug-location metadata, and the benefit of having a fully-formed dbg_value here seems fairly marginal.

[markus]

lib/CodeGen/TwoAddressInstructionPass.cpp

933 ↗

(On Diff #179818)

Of course if 'End == MBB->end()' after this point bad things would happen, but on the other hand that would also have been problematic in the original loop (and apparently it was not a problem in practice). Perhaps simply adding an assertion to indicate that this has been considered would be sufficient?

[aprantl]

lib/CodeGen/TwoAddressInstructionPass.cpp

933 ↗

(On Diff #179818)

The last instruction pretty much has to be a terminator instruction and can't really be a debug intrinsic.

llvm/trunk/lib/CodeGen/TwoAddressInstructionPass.cpp

else {
         Kills.push_back(MOReg);
     }
   }
 
   // Move the copies connected to MI down as well.
   MachineBasicBlock::iterator Begin = MI;
   MachineBasicBlock::iterator AfterMI = std::next(Begin);
   MachineBasicBlock::iterator End = AfterMI;
-  while (End->isCopy() &&
-         regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI)) {
+  while (End != MBB->end()) {
+    End = skipDebugInstructionsForward(End, MBB->end());
+    if (End->isCopy() && regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI))
       Defs.push_back(End->getOperand(0).getReg());
+    else
+      break;
     ++End;
   }
 
   // Check if the reschedule will not break dependencies.
   unsigned NumVisited = 0;
   MachineBasicBlock::iterator KillPos = KillMI;
   ++KillPos;
   for (MachineInstr &OtherMI : make_range(End, KillPos)) {

llvm/trunk/test/CodeGen/X86/twoaddr-dbg-value.mir

+# RUN: llc -run-pass=livevars,twoaddressinstruction -mtriple=x86_64-- -o - %s | FileCheck %s
+---
+name:            foo
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $edi
+
+    %0:gr32 = COPY killed $edi
+    %1:gr32 = COPY killed %0
+    %4:gr32 = XOR32ri8 %1, 1, implicit-def dead $eflags
+    DBG_VALUE %4
+    %5:gr32 = COPY %4
+    PUSH32r killed %1, implicit-def $esp, implicit $esp
+    $eax = COPY killed %5
+    RETQ implicit killed $eax
+
+...
+
+# Verify that the DBG_VALUE instruction does not inhibit
+# TwoAddressInstructionPass::rescheduleMIBelowKill optimization
+
+# CHECK: PUSH32r %1, implicit-def $esp, implicit $esp
+# CHECK-NEXT: %2:gr32 = COPY killed %1
+# CHECK-NEXT: %2:gr32 = XOR32ri8 %2, 1, implicit-def dead $eflags
+# CHECK-NEXT: DBG_VALUE %2
+# CHECK-NEXT: %3:gr32 = COPY killed %2