[PATCH] D91595: [Debugify] Support checking Machine IR debug info

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCheckDebug.cpp:58
+        for (MachineInstr &MI : MBB) {
+          if (!MI.isDebugValue())
+            continue;
----------------
djtodoro wrote:
> xiangzhangllvm wrote:
> > xiangzhangllvm wrote:
> > > djtodoro wrote:
> > > > I think we should do this for finding missing variables; but for the checking of missing lines we should check more machine instructions (especially the instrs other than debug ones), right? Any reason we shouldn't do that?
> > > > 
> > > > Please check the `checkDebugifyMetadata()` (from Debugify.cpp) how it was done.
> > > Because the MirDebugify will generate DBG_VALUE for every MIRs, all these MIRs' debug location will be saved in these generated DBG_VALUEs. So here we just need to check the line info in these DBG_VALUEs. It is also ok the check the non_debug MIR again, but it is duplicated.
> > Hi, @djtodoro 
> > Say it is "duplicated" may not accurate, here we only check the debug location in DBG_VALUE MIRs is more strict than check both of them. Because the optimizations may change/update the value of DBG_VALUE (its 1st operand), but they shouldn't change/remove the location info of it. And directly remove a DBG_VALUE should always report warning.
> On the IR level, debugify utility pass can tell us: //there is an instruction `I` with empty DebugLoc// after a pass. How it works: before a pass, it attaches synthetic dbg loc onto each IR instruction, and after the pass it checks if it preserved the dbg loc.
> 
> Can we do the same for MIR Debugify? For example: the mir-debugify attaches a dbgLoc on an MI (non debug one), and then the mir-check-debigify checks if a pass preserved that dbg location? Is it even possible at MIR level?
I agree with Djordje, the line numbers attached to DBG_VALUE instructions are of little interest and never reach an output file, it's the non-meta instructions that are the subject of optimisations, and thus should be the target of line number checkers.


================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:117-118
       unsigned Line = MI.getDebugLoc().getLine();
+      if (NumLines < Line)
+        NumLines = Line;
       if (!Line2Var.count(Line))
----------------
If I'm reading this correctly, won't NumLines always finish with the value `NextLine-1`, the counter used when setting line numbers above??


================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:123
       assert(LocalVar && "No variable for current line?");
+      VarSet.insert(LocalVar);
 
----------------
Couldn't this just be extracted from the Line2Var collection built earlier?


================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:143
 
+  // The new line number of MIR maybe more than the old line number of IR.
+  // So we save it into "llvm.mir.debugify", the variable number will not
----------------



================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:144
+  // The new line number of MIR maybe more than the old line number of IR.
+  // So we save it into "llvm.mir.debugify", the variable number will not
+  // changed and we directly save it into "llvm.mir.debugify" too.
----------------



================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:58
+      // And there's no meaning to add debug info for a terminator.
+      if (!MI.isTerminator())
+        MI.setDebugLoc(DILocation::get(Ctx, NextLine++, 1, SP));
----------------
xiangzhangllvm wrote:
> LuoYuanke wrote:
> > Why terminator instruction don't need debug info?
> because later LiveDebugVariables will drops out-of-liveness DBG_VALUEs, as the register allocator will not maintain the location of its vreg for positions that aren't live. 
> Pls refer https://github.com/llvm/llvm-project/blob/68ac02c0dd2b8fda52ac132a86f72f2ad6b139a5/llvm/lib/CodeGen/LiveDebugVariables.cpp#L657
> 
> 
Terminators probably shouldn't have variable location information attached to them. However this block is adding line numbers / source locations to instructions, and terminator line numbers can be useful (for example tail calls, or representing the end of a lexical block).


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

https://reviews.llvm.org/D91595