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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm 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.
 
----------------
vsk wrote:
> 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!
I'll fix it soon, thank you!


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:33
+
+    NamedMDNode *NMD = M.getNamedMetadata("llvm.mir.debugify");
+    if (!NMD) {
----------------
vsk wrote:
> 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').
There is 2 reasons: 
1) 'llvm.debugify' collected IR debug info (line + var number), if we "overwrite" the MIR debug info into it, it is confused to read it.
2) In fact, Machine debugify is called from (IR level) debugify. If we write the MIR debug info into 'llvm.debugify' metadata, it will be "overwrite" by IR debug info at last.


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:40
+
+    auto getDebugifyOperand = [&](unsigned Idx) -> unsigned {
+      return mdconst::extract<ConstantInt>(NMD->getOperand(Idx)->getOperand(0))
----------------
vsk wrote:
> 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?
en..., In fact, we intentionally make the logic same with IR debugify, it is reasonable to check MIR similar with IR. (It has already help me to find some debug info problems in some MIR optimizations).  It is trouble to reuse that facility in both the IR & MIR. The logic of MIR-debugify is some with  IR-debugify too, but MIR-debugify also implemented in a independent pass.


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

https://reviews.llvm.org/D91595