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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:346
+``MachineInstr`` in a ``Module``. And the MIR-level ``mir-check-debugify`` is
+similar with IR-level ``check-debugify`` pass.
 
----------------
xiangzhangllvm wrote:
> vsk wrote:
> > nit, s/with/to the/
> Hi @vsk Do you mean refine "is similar with IR-level" to "is similar to IR-level" ?
Yep!


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:33
+
+    NamedMDNode *NMD = M.getNamedMetadata("llvm.mir.debugify");
+    if (!NMD) {
----------------
xiangzhangllvm wrote:
> vsk wrote:
> > Why does this need to be named differently from the IR-level named debugify metadata?
> The MIR Check base on the MIR debugify, the "llvm.mir.debugify" will save the line number and variable number in MIR debugify.
> Please refer llvm/lib/CodeGen/MachineDebugify.cpp line 160,162
Yes, I understand that. What's not clear to me is why the named metadata needs to be different from the IR-level named metadata ('llvm.debugify').


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:40
+
+    auto getDebugifyOperand = [&](unsigned Idx) -> unsigned {
+      return mdconst::extract<ConstantInt>(NMD->getOperand(Idx)->getOperand(0))
----------------
xiangzhangllvm wrote:
> vsk wrote:
> > I don't think it's a good idea to introduce a copy of the IR-level check-debugify logic, since this makes it possible for the IR & MIR-level checking behaviors to diverge.
> > 
> > Can you find a way to factor out and reuse the checking logic? One way to do it might be to introduce a generic DebugifyChecker which parses the named metadata and operates on DebugLoc.
> It is not get/check IR-level metadata info, as explained before, here use the getDebugifyOperand to get "llvm.mir.debugify" info which generated by MIR debugify.
Hm, I don't think we're on the same page. My concern is that this patch copies logic from checkDebugifyMetadata (in lib/Transforms/Utils/Debugify.cpp), such as the logic to set up bitvectors to track missing lines. Could you please find a way to implement the warnings in a generic way, and reuse that facility in both the IR & MIR check-debugify passes?


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

https://reviews.llvm.org/D91595