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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

Nice! (few nits inline)



================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:9
+///
+/// \file This check debug info after mir-debugify (+ pass-to-test). Currently
+/// it simply check the integrity of line info in DILocation and DILocalVariable
----------------
checks


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:10
+/// \file This check debug info after mir-debugify (+ pass-to-test). Currently
+/// it simply check the integrity of line info in DILocation and DILocalVariable
+/// which mir-debugifiy generated before.
----------------
checks


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:65
+            continue;
+          }
+          if (!DL) {
----------------
Nit: please add \n here.


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:71
+          }
+        }
+        // Find missing variables.
----------------
Nit: please add \n here.




================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:58
+        for (MachineInstr &MI : MBB) {
+          if (MI.isDebugValue()) {
+            const DILocalVariable *LocalVar = MI.getDebugVariable();
----------------
xiangzhangllvm wrote:
> djtodoro wrote:
> > Not sure we want to report missing dbgLoc for meta instructions other than `DBG_VALUE` (e.g., `DBG_LABEL` does not end up in the final code as well..)
> For missing line, we should check all the MIRs which contribute the line number in MachineDebugify -- (non DBG_VALUEs), so here we just need to check MI.isDebugValue().
I see, but that should be fixed at `MachineDebugify` side first. At least, we can put a TODO (or FIXME) marker, saying //"Avoid meta instructions other than dbg_val" //


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

https://reviews.llvm.org/D91595