[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

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

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

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

12

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

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

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

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

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

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

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

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->isDebugInstr() ||
+         (End->isCopy() &&
+          regOverlapsSet(Defs, End->getOperand(1).getReg(), TRI))) {
+    if (!End->isDebugInstr())
       Defs.push_back(End->getOperand(0).getReg());
     ++End;
   }
 
   // Check if the reschedule will not break dependencies.
   unsigned NumVisited = 0;
   MachineBasicBlock::iterator KillPos = KillMI;
   ++KillPos;
   for (MachineInstr &OtherMI : make_range(End, KillPos)) {

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