[PATCH] D139590: [RegAllocFast] Handle new debug values for spills

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Apologies for taking a bit of time to get to this. My thoughts below, tl;dr it's probably better to just skip tracking spills for `DBG_VALUE_LIST`s in RegAllocFast.

The fix is reasonable in that it causes all of the debug operands to be processed for the `DBG_VALUE_LIST`, but from what I can see I think the issue is just that RegAllocFast does not handle debug info in optimized code well. Some of the behaviour results in incorrect behaviour in the presence of optimized code - it tracks the set of live debug values for a given register, but does account for earlier debug values being killed by later debug values, so produces potentially large bundles of redundant debug instructions (which unfortunately this fix would add to, as any newly produced debug values would be added to the list of debug values to repeatedly clone). Cloning debug values to the end of the basic block may also result in earlier debug values overwriting later debug values, producing incorrect debug info. This isn't normally a problem in unoptimized code, where we generally have a single `DBG_VALUE` for each variable and no `DBG_VALUE_LIST`s, which is what I believe RegAllocFast is normally expecting. It looks as though this is a somewhat unusual case in that the IR is optimized, but `llc` is being invoked with `-O0`; it's worth noting that `FastIsel` simply drops `DBG_VALUE_LIST`s outright without trying to produce an instruction.

If RegAllocFast is to handle these cases correctly then some effort could be put into fixing it by adding more support for tracking operands of debug values, but I don't think it would be worth it. Besides being somewhat outside of what RegAllocFast is intended to handle AFAIK, the problem of tracking logical values through machine locations throughout the program is non-trivial and should be solved in the LiveDebugValues pass as much as possible. For `DBG_VALUE` and `DBG_VALUE_LIST` instructions it is currently necessary for the register allocator to do some of this work in order to prevent certain debug values from being killed when we rewrite them to use a physical registers or spill slot, but this is more generally solved by the debug info instruction referencing work <https://llvm.org/docs/InstrRefDebugInfo.html>, which removes the need to track spills/restores prior to LiveDebugValues; support for multi-location debug values in `DBG_INSTR_REF` is not currently present but is being worked on and due to land soon, which should resolve this issue if instruction referencing is enabled for the build in question.



================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:451-467
   for (auto MISpilledOperands : SpilledOperandsMap) {
     MachineInstr &DBG = *MISpilledOperands.first;
     MachineInstr *NewDV = buildDbgValueForSpill(
         *MBB, Before, *MISpilledOperands.first, FI, MISpilledOperands.second);
     assert(NewDV->getParent() == MBB && "dangling parent pointer");
-    (void)NewDV;
     LLVM_DEBUG(dbgs() << "Inserting debug info due to spill:\n" << *NewDV);
+    handleDebugValue(*NewDV);
----------------
See full review comment. `DBG_VALUE_LIST` still needs to be handled in `handleDebugValue` to ensure its operands are updated correctly, but this change means that it won't be updated through spills.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139590