[DebugInfo][InstrRef] Don't break up return-sequences on debug-info instructions

[jmorse]

Summary

When we have a terminator sequence (i.e. a tailcall or return), MIIsInTerminatorSequence is used to work out where the preceding ABI-setup instructions end, i.e. the parts that were glued to the terminator instruction. This allows LLVM to split blocks safely without having to worry about ABI stuff.

The function only ignores DBG_VALUE instructions, meaning that the two debug instructions I recently added can end terminator sequences early, causing various MachineVerifier errors. This patch promotes the test for debug instructions from "isDebugValue" to "isDebugInstr", thus avoiding any debug-info interfering with this function.

Unfortunately I don't have a test where I can replicate this: it would require getting ScheduleDAGSDNodes to emit debug instrs between a tail call and the instrs glued to it. This is happening on a large codebase (that I can't directly access) deep inside LTO, and this change fixes it. Hopefully everyone agrees: this is an obvious fix.

[djtodoro]

Can we make a test?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1663

Should it be isMetaInstruction() ?

[jmorse]

Can we make a test?

Awkwardly, I'm not sure -- for some IR that looks like this, with function attribute sspstrong:

musttail call void %65(%foo *%call.i.i28.i.i.i, %bar* %60, i32 %63, i32 255), !dbg !12
ret void, !dbg !12

We usually get the following MIR after SelectionDAG runs, with an additional stack safety check put in before the tail call, to abort if the stack has overflowed,

  %151:gr64 = MOV64rm [Load of stack canary value]
  %152:gr64 = SUB64rm %151:gr64(tied-def 0), %stack.0.StackGuardSlot1, 1, $noreg, 0, $noreg, implicit-def $eflags
  JCC_1 %bb.78, 5, implicit $eflags
  JMP_1 %bb.40

bb.40.SP_return:
  %153:gr32 = MOV32ri 255
  $rdi = COPY %25:gr64, debug-location !12; foo.cpp:10
  $rsi = COPY %24:gr64, debug-location !12; foo.cpp:10
  $edx = COPY %26:gr32, debug-location !12; foo.cpp:10
  $ecx = COPY %153:gr32, debug-location !12; foo.cpp:10
  TCRETURNri64 %27:gr64_tc, 0, <regmask>, implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, implicit $edx, implicit $ecx, debug-location !12; foo.cpp:10

Normally the modified function allows LLVM to identify that all the physreg are part of the tail-call-termination sequence. However if you put a DBG_INSTR_REF or DBG_PHI immediately in front of the TCRETURNri64, it breaks up the sequence of instructions in a way that the MachineVerifier complains about.

I'm not really sure how a debug instruction gets put between the TCRETURNri64 and the COPYs, there aren't any new values defined in that sequence, and those COPYs are glued to the return instruction by SelectionDAG. There are some comments in ScheduleDAGSDNodes.cpp about dropping loose DBG_VALUEs before the final terminator, I'll try a build with a breakpoint stuck in there, but I'm not hopeful.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1663

The implications of this are unclear to me -- the "KILL" instruction for example manually edits register liveness information, and I'm not sure where in a termination sequence it would belong. IMO, best to keep the change so as small an amount of behaviours as possible, hence picking isDebugInstr.

[dblaikie]

(I wouldn't insist this is held up until a test is added - but I've got a strong preference for a test to be added. If it's hard to find a case, perhaps adding an assertion (faster than a breakpoint/running the whole thing under a debugger) isDebugValue || !isDebugInstr I think would be the interesting property to assert - and then hopefully running that over a codebase should yield something, then creduce/llvm-reduce checking for that assertion might reduce out to something useful?)

[jmorse]

Various stage2reldeb builds of things didn't reproduce this, but I've managed to mangle a testcase (CodeGen/ARM/dbg-tcreturn.ll) to do what I want. It turns out the key interactions are:

• IR operation transformed into a call,
• Call optimised into a tail call,

Which is what smuggles a debug instruction to the end of the block without other guards catching it. After that, sspstrong is needed to trigger stack check insertion, which then mis-interprets the DBG_INSTR_REF.

[Orlando]

LGTM with the new test which appears to be the only previously outstanding request.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1663

SGTM. I just want to check that @djtodoro is happy with this too?

[djtodoro]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

1663

All good. I agree with Jeremy that other meta instructions shouldn't affect this piece of code.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

 static bool MIIsInTerminatorSequence(const MachineInstr &MI) {
   // If we do not have a copy or an implicit def, we return true if and only if
   // MI is a debug value.
   if (!MI.isCopy() && !MI.isImplicitDef())
     // Sometimes DBG_VALUE MI sneak in between the copies from the vregs to the
     // physical registers if there is debug info associated with the terminator
     // of our mbb. We want to include said debug info in our terminator
     // sequence, so we return true in that case.
-    return MI.isDebugValue();
+    return MI.isDebugInstr();
 
   // We have left the terminator sequence if we are not doing one of the
   // following:
   //
   // 1. Copying a vreg into a physical register.
   // 2. Copying a vreg into a vreg.
   // 3. Defining a register via an implicit def.
 

llvm/test/DebugInfo/ARM/instr-ref-tcreturn.ll

+; RUN: llc %s -o - -stop-after=finalize-isel -verify-machineinstrs -experimental-debug-variable-locations | FileCheck %s
+
+; In the sequence below, the sdiv is converted to a function call to __divsi3,
+; which is then tail call optimised. The dbg.value is suddenly stuck between
+; terminators, and the corresponding DBG_INSTR_REF is forced-placed to be
+; immediately before the TCRETURN.
+; However, with the function having the sspstrong attribute, we then try to
+; peel apart the terminator sequence, DBG_INSTR_REF is interpreted as being
+; a "real" instruction, and the stack check is inserted at that point rather
+; than before the copies-to-physreg setting up the call. This breaks the
+; code, and MachineVerifier complains.
+;
+; Check that the tail sequence is stack-protected, and split at the correct
+; position, ignoring the DBG_INSTR_REF
+
+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:  bb.0.entry:
+; CHECK:        LOAD_STACK_GUARD
+
+; CHECK-LABEL:  bb.2.entry:
+; CHECK:        tBL {{.*}} &__stack_chk_fail,
+
+; CHECK-LABEL:  bb.1.entry:
+; CHECK:          $r0 = COPY %0
+; CHECK-NEXT:     $r1 = COPY %1
+; CHECK-NEXT:     DBG_INSTR_REF 1, 0
+; CHECK-NEXT:     TCRETURNdi &__divsi3, 0, implicit $sp, implicit $r0, implicit $r1
+
+declare i1 @ext()
+
+define i32 @test(i32 %a1, i32 %a2) #1 !dbg !5 {
+entry:
+  %foo = alloca i32, i32 %a1
+  %bool = call i1 @ext()
+  %res = sdiv i32 %a1, %a2
+  call void @llvm.dbg.value(metadata i32 %a1, metadata !13, metadata !DIExpression()), !dbg !16
+  ret i32 %res
+}
+
+attributes #1 = {sspstrong}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !1, producer: "Swift", isOptimized: true, runtimeVersion: 5, emissionKind: FullDebug)
+!1 = !DIFile(filename: "foo.swift", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"Swift Minor Version", i8 3}
+!5 = distinct !DISubprogram(name: "n0", linkageName: "n1", scope: !7, file: !6, line: 86, type: !8, scopeLine: 86, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!6 = !DIFile(filename: "bar.swift", directory: "")
+!7 = !DIModule(scope: null, name: "Swift")
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !12}
+!10 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Int", scope: !7, file: !11, size: 32, elements: !2, runtimeLang: DW_LANG_Swift, identifier: "$i1")
+!11 = !DIFile(filename: "f1.swift", directory: "")
+!12 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "n2", scope: !7, file: !6, size: 32, elements: !2, runtimeLang: DW_LANG_Swift, identifier: "n3")
+!13 = !DILocalVariable(name: "n4", scope: !14, file: !1, line: 89, type: !15)
+!14 = distinct !DILexicalBlock(scope: !5, file: !6, line: 86, column: 34)
+!15 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !10)
+!16 = !DILocation(line: 89, column: 9, scope: !14)