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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
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:
> > xiangzhangllvm wrote:
> > > vsk wrote:
> > > > 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.
> > > I thought your point is: reuse the same code (Debugify.cpp: checkDebugifyMetadata) to check both IR and MIR. right?
> > > The troubles are 
> > > 1) We must complex current clear checkDebugifyMetadata to fit the MIR check,(the line/variable numbers are different, the IR/MIR iterator is different, all need to fit) and call relations. What's more, Current key code in mir-check-debugify just tens of lines, very small.
> > > 2)Current mir-check-debugify is a simple and dependent pass, easy to extend later, if reuse the Debugify.cpp code, it at last need to call IR-level debugify as mir-debugify. (We mostly use -run-pass=mir-check-debugify to run it.)
> > >The logic of MIR-debugify is same with IR-debugify too
> > Both of them mainly add debug IR/MIR for very instructions, and MIR-debugify really called IR-debugify's code, but it just want to use IR-debugify to create metadata.
> > I thought your point is: reuse the same code (Debugify.cpp: checkDebugifyMetadata) to check both IR and MIR. right?
> 
> Ah, no, not the exact same code, no. There are certain similarities between the checking logic in checkDebugifyMetadata, and the checking logic introduced in this patch. Instead of having two copies of the same checking logic that can diverge over time, I'm suggesting that you factor out the similar pieces into a single generic implementation. The parts of the logic that are IR or MIR specific, of course, can/should remain separate.
Yes, merging same logic is the general rule of coding.
But if it makes things complex or down the readability, I prefer not merging.
Currently, both IR/MIR check is very simple and clear, merging won't take profit. If we do merging, we at least need to do:
1) Adding function call, let both passes can call it.
1) Adding pass Line/variable numbers and others to the function.
2) Distinguish IR/MIR (Function/MachineFunction, BB/MBB, I/MI) in the function.
3) Distinguish "isa<DbgValueInst>(&I)" with "MI.isDebugValue()" and so on.



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

https://reviews.llvm.org/D91595