[ScheduleDAG] Move DBG_VALUEs after first term forward.

[fhahn]

Summary

MBBs are not allowed to have non-terminator instructions after the first
terminator. Currently in some cases (see the modified test),
EmitSchedule can add DBG_VALUEs after the last terminator, for example
when referring a debug value that gets folded into a TCRETURN
instruction on ARM.

This patch updates EmitSchedule to move inserted DBG_VALUEs just before
the first terminator. I am not sure if there are terminators produce
values that can in turn be used by a DBG_VALUE. In that case, moving the
DBG_VALUE might result in referencing an undefined register. But in any
case, it seems like currently there is no way to insert a proper DBG_VALUEs
for such registers anyways.

Alternatively it might make sense to just remove those extra DBG_VALUES.

I am not too familiar with the details of debug info in the backend and
would appreciate any suggestions on how to address the issue in the best
possible way.

[paquette]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

1037–1060

Does this ever happen with anything other than tail calls?

[fhahn]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

1037–1060

I am not sure. I think the reason this happens here is that the instruction with the debug value attached (the sdiv) gets folded with the return instruction into TCRETURNdi during SelectionDAG. And now the DBG_VALUE is attached to the TCRETURN.

[jmorse]

(Drive-by review) Thanks for the patch -- it looks good, and generating legal machine code is definitely better than illegal,

it seems like currently there is no way to insert a proper DBG_VALUEs for such registers anyways.

I agree with this -- IMHO, this patch should also make any moved DBG_VALUE an undef / $noreg DBG_VALUE. If the DBG_VALUE somehow refers to a register defined by the first terminator, then it'll definitely be wrong after being moved, and will be clobbered on the next instruction anyway. All other DBG_VALUEs can't have been attached to the terminator node, and will have been emitted before the first terminator (lines 986 of this patch to 996).

(Shameless plug -- using instruction referencing instead as per http://lists.llvm.org/pipermail/llvm-dev/2020-February/139440.html was largely inspired by this EmitSchedule method, which is tricky to get right).

[fhahn]

In D83561#2151128, @jmorse wrote:

(Drive-by review) Thanks for the patch -- it looks good, and generating legal machine code is definitely better than illegal,

Thanks, very much appreciated.

it seems like currently there is no way to insert a proper DBG_VALUEs for such registers anyways.

I agree with this -- IMHO, this patch should also make any moved DBG_VALUE an undef / $noreg DBG_VALUE. If the DBG_VALUE somehow refers to a register defined by the first terminator, then it'll definitely be wrong after being moved, and will be clobbered on the next instruction anyway. All other DBG_VALUEs can't have been attached to the terminator node, and will have been emitted before the first terminator (lines 986 of this patch to 996).

Sounds good to me. Is there an easy way to do so?

[jmorse]

Should be a matter of

MI.getOperand(0).setReg(0)

Operand zero is the variable location; zero / $noreg is used to indicate that there is no location for the variable. If it's not a register MachineOperand, the ChangeToRegister method would be needed.

[fhahn]

In D83561#2153981, @jmorse wrote:

Should be a matter of

MI.getOperand(0).setReg(0)

Operand zero is the variable location; zero / $noreg is used to indicate that there is no location for the variable. If it's not a register MachineOperand, the ChangeToRegister method would be needed.

Thanks, updated to use ChangeToRegister(0, false).

[aprantl]

llvm/include/llvm/CodeGen/MachineInstr.h

283

///

llvm/include/llvm/CodeGen/MachineInstr.h

public:
   MachineInstr(const MachineInstr &) = delete;
   MachineInstr &operator=(const MachineInstr &) = delete;
   // Use MachineFunction::DeleteMachineInstr() instead.
   ~MachineInstr() = delete;
 
   const MachineBasicBlock* getParent() const { return Parent; }
   MachineBasicBlock* getParent() { return Parent; }
 
+  /// Move the instruction before \p MovePos.
+  void moveBefore(MachineInstr *MovePos);
+
   /// Return the function that contains the basic block that this instruction
   /// belongs to.
   ///
   /// Note: this is undefined behaviour if the instruction does not have a
   /// parent.
   const MachineFunction *getMF() const;
   MachineFunction *getMF() {
     return const_cast<MachineFunction *>(

llvm/lib/CodeGen/MachineInstr.cpp

MachineInstr::MachineInstr(MachineFunction &MF, const MachineInstr &MI)
   // Copy operands.
   for (const MachineOperand &MO : MI.operands())
     addOperand(MF, MO);
 
   // Copy all the sensible flags.
   setFlags(MI.Flags);
 }
 
+void MachineInstr::moveBefore(MachineInstr *MovePos) {
+  MovePos->getParent()->splice(MovePos, getParent(), getIterator());
+}
+
 /// getRegInfo - If this instruction is embedded into a MachineFunction,
 /// return the MachineRegisterInfo object for the current function, otherwise
 /// return null.
 MachineRegisterInfo *MachineInstr::getRegInfo() {
   if (MachineBasicBlock *MBB = getParent())
     return &MBB->getParent()->getRegInfo();
   return nullptr;
 }

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

for (const auto &InstrOrder : Orders) {
       if (DLI == DLE)
         break;
 
       LastOrder = Order;
     }
   }
 
   InsertPos = Emitter.getInsertPos();
-  return Emitter.getBlock();
+  // In some cases, DBG_VALUEs might be inserted after the first terminator,
+  // which results in an invalid MBB. If that happens, move the DBG_VALUEs
+  // before the first terminator.
+  MachineBasicBlock *InsertBB = Emitter.getBlock();
+  auto FirstTerm = InsertBB->getFirstTerminator();
+  if (FirstTerm != InsertBB->end()) {
+    assert(!FirstTerm->isDebugValue() &&
+           "first terminator cannot be a debug value");
+    for (MachineInstr &MI : make_early_inc_range(
+             make_range(std::next(FirstTerm), InsertBB->end()))) {
+      if (!MI.isDebugValue())
+        continue;
+
+      if (&MI == InsertPos)
+        InsertPos = std::prev(InsertPos->getIterator());
+
+      // The DBG_VALUE was referencing a value produced by a terminator. By
+      // moving the DBG_VALUE, the referenced value also needs invalidating.
+      MI.getOperand(0).ChangeToRegister(0, false);
+      MI.moveBefore(&*FirstTerm);
+    }
+  }
+  return InsertBB;
 }
 
 /// Return the basic block label.
 std::string ScheduleDAGSDNodes::getDAGName() const {
   return "sunit-dag." + BB->getFullName();
 }

llvm/test/CodeGen/ARM/dbg-tcreturn.ll

-; XFAIL: *
-; RUN: llc %s -o - -stop-after=finalize-isel -verify-machineinstr | FileCheck %s
+; RUN: llc %s -o - -stop-after=finalize-isel -verify-machineinstrs | FileCheck %s
 
 target datalayout = "e-m:o-p:32:32-Fi8-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
 target triple = "thumbv7-apple-ios7.0.0"
 
 ; CHECK-LABEL: name: test
 ; CHECK:       body:
 ; CHECK-NEXT:   bb.0.entry:
 ; CHECK-NEXT:     liveins: $r0, $r1
 ; CHECK:          %1:gpr = COPY $r1
 ; CHECK-NEXT:     %0:gpr = COPY $r0
 ; CHECK-NEXT:     $r0 = COPY %0
 ; CHECK-NEXT:     $r1 = COPY %1
-; CHECK-NEXT:     TCRETURNdi &__divsi3, implicit $sp, implicit $r0, implicit $r1
 ; CHECK-NEXT:     DBG_VALUE $noreg, $noreg, !13, !DIExpression(), debug-location !16
+; CHECK-NEXT:     TCRETURNdi &__divsi3, implicit $sp, implicit $r0, implicit $r1
 
 define i32 @test(i32 %a1, i32 %a2) !dbg !5 {
 entry:
   %res = sdiv i32 %a1, %a2
   call void @llvm.dbg.value(metadata i32 %res, metadata !13, metadata !DIExpression()), !dbg !16
   ret i32 %res
 }