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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/lib/CodeGen/CMakeLists.txt:77
   MachineCSE.cpp
+  MachineCheckDebug.cpp
   MachineDebugify.cpp
----------------
xiangzhangllvm wrote:
> xiangzhangllvm wrote:
> > djtodoro wrote:
> > > I would pick `MachineCheckDebugify`, wdyt?
> > Yes, I noticed that, I also found mir-strip-debug implemented in MachineStripDebug.cpp, so I sync it with mir-strip-debug.
> If you still prefer MachineCheckDebugify, I am ok to update it, include the option name.
But there is still MachineDebugify.cpp, so I think MachineCheckDebugify.cpp is more appropriate name.


================
Comment at: llvm/lib/CodeGen/MachineCheckDebug.cpp:58
+        for (MachineInstr &MI : MBB) {
+          if (!MI.isDebugValue())
+            continue;
----------------
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?


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

https://reviews.llvm.org/D91595