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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:33
+
+    NamedMDNode *NMD = M.getNamedMetadata("llvm.mir.debugify");
+    if (!NMD) {
----------------
xiangzhangllvm wrote:
> 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.
Ah, got it. The reason the named metadata nodes are kept separate is so the IR in a .mir file can be checked, and so there's no confusion about whether the metadata describes IR/MIR.

One small clarification -- it looks like it is the other way around: MachineDebugify calls the IR-level debugify (in DebugifyMachineModule::runOnModule).


================
Comment at: llvm/lib/CodeGen/MachineCheckDebugify.cpp:40
+
+    auto getDebugifyOperand = [&](unsigned Idx) -> unsigned {
+      return mdconst::extract<ConstantInt>(NMD->getOperand(Idx)->getOperand(0))
----------------
xiangzhangllvm wrote:
> xiangzhangllvm wrote:
> > 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.
> > 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.
> 
> 
I think we're still talking a bit past each other. Could you elaborate on this point:

>  It is trouble to reuse that facility in both the IR & MIR. The logic of MIR-debugify is some with IR-debugify too

The way I see it, the IR-level and MIR-level checking is very similar. I'm simply asking for the similar pieces to be factored out into a generic implementation.


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

https://reviews.llvm.org/D91595