Page MenuHomePhabricator

[ScheduleDAG] Move DBG_VALUEs after first term forward.
ClosedPublic

Authored by fhahn on Jul 10 2020, 8:38 AM.

Details

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.

Diff Detail

Event Timeline

fhahn created this revision.Jul 10 2020, 8:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
paquette added inline comments.Mon, Jul 13, 11:08 AM
llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
1037–1060

Does this ever happen with anything other than tail calls?

fhahn marked an inline comment as done.Mon, Jul 13, 12:37 PM
fhahn added inline comments.
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 added a subscriber: jmorse.Tue, Jul 14, 11:31 AM

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

(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 added a comment.EditedWed, Jul 15, 11:34 AM

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 updated this revision to Diff 278485.Thu, Jul 16, 8:09 AM

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 accepted this revision.Thu, Jul 16, 3:40 PM
aprantl added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
283

///

This revision is now accepted and ready to land.Thu, Jul 16, 3:40 PM
This revision was automatically updated to reflect the committed changes.