[PATCH] D52614: [DebugInfo][Dexter] Divide-before-return displays wrong value in debugger

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso marked 10 inline comments as done.
CarlosAlbertoEnciso added inline comments.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:1547
 
+  /// Update any matching DBG_VALUEs with the given register.
+  void updateDebugValues(unsigned Reg);
----------------
CarlosAlbertoEnciso wrote:
> aprantl wrote:
> > How about:
> > 
> > `/// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.`
> > 
> > Looking at the implementation of collectDebugValues() I'm somewhat puzzled how it works. Perhaps I'm misunderstanding what it is supposed to do, please let me know. It's scanning for DBG_VALUEs that point to Op0 of the instruction. Is Op0 always going to be a def? Or should it be improved?
> > /// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.
> 
> That is a better description.
> Looking at the implementation of collectDebugValues() I'm somewhat puzzled how it works. Perhaps I'm misunderstanding what it is supposed to do, please let me know. It's scanning for DBG_VALUEs that point to Op0 of the instruction.

It scans all the MI that follows the given instruction (they are referenced by 'DI').

```
    if (!DI->isDebugValue())
      return;
```
Returns when the scanned DI is not a DBG_VALUE

```
    if (DI->getOperand(0).isReg() &&
        DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
      DbgValues.push_back(&*DI);
```
Records the scanned DI (which already we know is DBG_VALUE), only if:

* The DI machine operand type is a 'register operand' and
* The DI register number is the same as the MI register number.

>  Is Op0 always going to be a def?

As far as I understand it, only for the case of DBG_VALUE, the Op0 should always be a def.

>  Or should it be improved?

Only the first operand in the MI (getOperand(0)) is considered, which I can see as a possible limitation for cases where a different operand register needs to be modified.

And if the assumption that Op0 is not always a def, then extra checks should be added.



================
Comment at: include/llvm/CodeGen/MachineInstr.h:1548
+  /// Update any matching DBG_VALUEs with the given register.
+  void updateDebugValues(unsigned Reg);
+
----------------
aprantl wrote:
> Could you give this a more descriptive name?
What about: `changeDebugValuesDefReg`

meaning

Changes the register definition for the DBG_VALUEs to the given 'register'.


Repository:
  rL LLVM

https://reviews.llvm.org/D52614