[PATCH] D52614: [DebugInfo][Dexter] Incorrect DBG_VALUE after MCP dead copy instruction removal.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:1547
 
+  /// Update any matching DBG_VALUEs with the given register.
+  void updateDebugValues(unsigned Reg);
----------------
CarlosAlbertoEnciso wrote:
> 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.
> 
Oh I see. MI is always a DBG_VALUE. Then looking at Op0 makes perfect sense. Thanks for clearing that up!


================
Comment at: test/CodeGen/MIR/X86/pr38773.mir:153
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    DBG_VALUE debug-use $ecx, debug-use $noreg, !14, !DIExpression(), debug-location !23
----------------
CarlosAlbertoEnciso wrote:
> CarlosAlbertoEnciso wrote:
> > aprantl wrote:
> > > This line is an exact duplicate of the previous line. While this won't break anything, it's inefficient.
> > > Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix. Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.
> > The duplicated line has been removed. Modified the IR just to include only a single DBG_VALUE.
> > Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix.
> 
> If you are OK, I can leave that issue to be fix in another patch. 
> 
> > Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.
> 
> That is a very good point.
> 
> During the review [[ https://reviews.llvm.org/D50887 | D50887 ]], there were some discussions about the `collectDebugValues` implementation. The idea was to use the list of USEs instead of linearly scanning.
> 
> Another interesting modification, was the one you raised on [[ https://bugs.llvm.org/show_bug.cgi?id=38763#c8 | PR38763#c8 ]] to extended llvm.dbg.value to take more than one LLVM SSA Value.
> 
> I would suggest if we can create specific bugzillas to address the following issues:
> 
> * `collectDebugValues` implementation (to use the list of USEs).
> * Check for duplicates when inserting DBG_VALUEs.
> * Extended llvm.dbg.value to take more than one LLVM SSA Value.
> 
> I am happy to create those bugzillas, so we can have something in concrete to work on.
>I am happy to create those bugzillas, so we can have something in concrete to work on.
Please do. Working in small increments makes it much easier to reason over patches and also makes reviews much faster!


https://reviews.llvm.org/D52614