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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1663
     // sequence, so we return true in that case.
-    return MI.isDebugValue();
+    return MI.isDebugInstr();
 
----------------
djtodoro wrote:
> Should it be `isMetaInstruction()` ?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106660/new/

https://reviews.llvm.org/D106660