[PATCH] D53657: [DEBUG_INFO][NVPTX]Fix processing of DBG_VALUES.

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: lib/Target/NVPTX/NVPTXRegisterInfo.cpp:122-129
+  int Offset = MF.getFrameInfo().getObjectOffset(FrameIndex);
 
   // Using I0 as the frame pointer
-  MI.getOperand(FIOperandNum).ChangeToRegister(NVPTX::VRFrame, false);
+  MI.getOperand(FIOperandNum).ChangeToRegister(NVPTX::VRFrame, /*isDef=*/false);
+  // Special processing of dbg_value instructions.
+  if (!MI.isDebugValue())
+    Offset += MI.getOperand(FIOperandNum + 1).getImm();
----------------
ABataev wrote:
> tra wrote:
> > I'm not sure I understand this change.  What is in getOperand(FIOperandNum + 1) for DebugValue instructions? Other instructions carry FI-based offset, but your change appears to ignore it and effectively treats it as if it's always zero.
> > 
> > It may be worth adding more details about *why* debug values need to be handled this way.
> > 
> DebugValue does not have FI-based offset. That's why it causes compiler crash. For DebugValue it is enough just to have the base object offset. 
> Some other targets, like ARC, Thumb have same kind of processing.
> Alternatively, we can use the code similar to one existing in `lib/CodeGen/PrologEpilogInserter.cpp` for special handling of the debug values:
> ```
>       // Frame indices in debug values are encoded in a target independent
>       // way with simply the frame index and offset rather than any
>       // target-specific addressing mode.
>       if (MI.isDebugValue()) {
>         assert(i == 0 && "Frame indices can only appear as the first "
>                          "operand of a DBG_VALUE machine instruction");
>         unsigned Reg;
>         int64_t Offset =
>             TFI->getFrameIndexReference(MF, MI.getOperand(0).getIndex(), Reg);
>         MI.getOperand(0).ChangeToRegister(Reg, false /*isDef*/);
>         MI.getOperand(0).setIsDebug();
>         auto *DIExpr = DIExpression::prepend(MI.getDebugExpression(),
>                                              DIExpression::NoDeref, Offset);
>         MI.getOperand(3).setMetadata(DIExpr);
>         continue;
>       }
> 
> ```
OK.  Adding the comment that DebugValue uses different encoding convention that does not need in-object offset should do.

Perhaps the intention could be made a bit more obvious if the code was structured like this:

```
// Explain why DebugValue has InObjectOffset = 0.
InObjectOffset = MI.isDebugValue() ? 0 : MI.getOperand(FIOperandNum + 1).getImm()
Offset = MF.getFrameInfo().getObjectOffset(FrameIndex) + InObjectOffset;
```




Repository:
  rL LLVM

https://reviews.llvm.org/D53657