[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/MachineCheckDebug.cpp:58
+        for (MachineInstr &MI : MBB) {
+          if (!MI.isDebugValue())
+            continue;
----------------
jmorse wrote:
> djtodoro wrote:
> > xiangzhangllvm wrote:
> > > xiangzhangllvm wrote:
> > > > djtodoro wrote:
> > > > > I think we should do this for finding missing variables; but for the checking of missing lines we should check more machine instructions (especially the instrs other than debug ones), right? Any reason we shouldn't do that?
> > > > > 
> > > > > Please check the `checkDebugifyMetadata()` (from Debugify.cpp) how it was done.
> > > > Because the MirDebugify will generate DBG_VALUE for every MIRs, all these MIRs' debug location will be saved in these generated DBG_VALUEs. So here we just need to check the line info in these DBG_VALUEs. It is also ok the check the non_debug MIR again, but it is duplicated.
> > > Hi, @djtodoro 
> > > Say it is "duplicated" may not accurate, here we only check the debug location in DBG_VALUE MIRs is more strict than check both of them. Because the optimizations may change/update the value of DBG_VALUE (its 1st operand), but they shouldn't change/remove the location info of it. And directly remove a DBG_VALUE should always report warning.
> > On the IR level, debugify utility pass can tell us: //there is an instruction `I` with empty DebugLoc// after a pass. How it works: before a pass, it attaches synthetic dbg loc onto each IR instruction, and after the pass it checks if it preserved the dbg loc.
> > 
> > Can we do the same for MIR Debugify? For example: the mir-debugify attaches a dbgLoc on an MI (non debug one), and then the mir-check-debigify checks if a pass preserved that dbg location? Is it even possible at MIR level?
> I agree with Djordje, the line numbers attached to DBG_VALUE instructions are of little interest and never reach an output file, it's the non-meta instructions that are the subject of optimisations, and thus should be the target of line number checkers.
Hello @jmorse  @djtodoro, first thanks for your comments, I was thinking about it before, and it is nice discuss with you now:
**What we really want (MIR/IR)check-debug doing for us** ?
Take (IR)check-debug for e.g. 
In my eyes, we use check-debug to check if the optimizations **incorrectly** optimized the debug info of IRs. Right?
Some optimizations may **correctly** optimized the IRs by removing them with carefully handling its debug info. (e.g. --instsimplify), for these cases, I think, check-debug should not report WARNING messages out.

Take llvm lit test llvm/test/DebugInfo/debugify.ll for e.g.
There is unused %sum=... in it, let's remove it in instruction simplify.
After debugify, it will be:

```
define i32 @bar() !dbg !12 {
  call void @foo(), !dbg !15
  %sum = add i32 0, 1, !dbg !16
  call void @llvm.dbg.value(metadata i32 %sum, metadata !14, metadata !DIExpression()), !dbg !16
  ret i32 0, !dbg !17
}
```
If I run 

```
opt -debugify --instsimplify -check-debugify -S -o - DebugInfo/debugify.ll
```
it will report WARNING in -check-debugify:

```
WARNING: Missing line 3
CheckModuleDebugify: PASS
......
define i32 @bar() !dbg !12 {
  call void @foo(), !dbg !15
  call void @llvm.dbg.value(metadata i32 0, metadata !14, metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value)), !dbg !16
  ret i32 0, !dbg !17
}

```
because the check-debug will check the line info of removed instruction.
What is more, removing instruction is very common in optimization passes, and debugify will add different line info for every instructions, so once remove any non-debug instructions, it will give out WARNINGs. Even they **correctly** remove them (by updating its llvm.dbg.value (0/undef, ....).

If the test is big, we may need to check a lot of warnings one by one.

And that was why my current patch didn't check the line info for non-debug MIRs.



================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:117-118
       unsigned Line = MI.getDebugLoc().getLine();
+      if (NumLines < Line)
+        NumLines = Line;
       if (!Line2Var.count(Line))
----------------
jmorse wrote:
> If I'm reading this correctly, won't NumLines always finish with the value `NextLine-1`, the counter used when setting line numbers above??
Here is really risk:
I write it here because NextLine-1 may less than line in DILocation generated at (IR)debugify before.
Line 120 update the line from IR (not MIR).
I'll refine here, tks!


================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:123
       assert(LocalVar && "No variable for current line?");
+      VarSet.insert(LocalVar);
 
----------------
jmorse wrote:
> Couldn't this just be extracted from the Line2Var collection built earlier?
Yes, but seems same, and doesn't it is better here? -- the life range of Varset is short.


================
Comment at: llvm/lib/CodeGen/MachineDebugify.cpp:143
 
+  // The new line number of MIR maybe more than the old line number of IR.
+  // So we save it into "llvm.mir.debugify", the variable number will not
----------------
jmorse wrote:
> 
I'll fix them, thank you!


================
Comment at: llvm/test/CodeGen/Generic/MIRDebugify/check-line-and-variables.ll:1
+; RUN: llc -debugify-and-check-strip-all-safe -o - %s 2>&1 | FileCheck %s
+
----------------
MaskRay wrote:
> Does this file add new coverage?
> 
> This file explain its purpose in the file header. The body of foo is mostly unused, you can add just a `ret`. If you don't otherwise check the output, use `-filetype=null`
Terminator IR ret is too simple to check the debug info.
Currently I check two MIR pass will pass this debug info check.
And I think it is better for this *.ll file to keep "same" with its MIR file check-line-and-variables.mir.
tks!


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

https://reviews.llvm.org/D91595