[PATCH] D82363: [DebugInfo] Add new instruction and expression operator for variadic debug values

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:432
+      MachineOperand &MO = DBG->getDebugOperand(0);
+      if (MO.isReg() && MO.getReg() == 0) {
+        updateDbgValueForSpill(*DBG, FI, 0);
----------------
jmorse wrote:
> Is this rewriting
>   DBG_VALUE $noreg
> to
>   DBG_VALUE %stack.0
> ?
> 
> Obviously this patch just preserves old behaviour, which is fine, but I get the feeling that the old behaviour is wrong.
I'll have to run through RegAllocFast again to be sure of the full behaviour here... as far as I understand, the intent here is that if the DBG_VALUE used the spilled virtual register but was unassigned for some reason in this pass, then we recover it using the spilled value here. I think that that could be valid, but I don't understand why the location would be set to `$noreg` if the virtual register hasn't been totally removed yet (which it shouldn't be if it's being spilled here).


================
Comment at: llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp:84-87
+            SmallVector<uint64_t, 8> Ops;
+            DIExpression::appendOffset(Ops, Offset);
+            unsigned OpIdx = MI.getDebugOperandIndex(&MI.getOperand(i));
+            DIExpr = DIExpression::appendOpsToArg(DIExpr, Ops, OpIdx);
----------------
jmorse wrote:
> Should this be using the new "getOffsetOpcodes" hook?
Probably - the existing code isn't, but I'm happy to correct it for the "new" case.


================
Comment at: llvm/lib/Target/SystemZ/SystemZRegisterInfo.cpp:280-286
+      unsigned OpIdx = MI->getDebugOperandIndex(&MI->getOperand(FIOperandNum));
+      SmallVector<uint64_t, 3> Ops;
+      DIExpression::appendOffset(
+          Ops, TFI->getFrameIndexReference(MF, FrameIndex, BasePtr).getFixed());
+      MI->getDebugExpressionOp().setMetadata(
+          DIExpression::appendOpsToArg(MI->getDebugExpression(), Ops, OpIdx));
+    }
----------------
jmorse wrote:
> Should this be using the new "getOffsetOpcodes" hook?
Ditto the above response, old code isn't calling it but we can fix it as part of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363