[PATCH] D69889: [DebugInfo] Avoid creating entry values for clobbered registers

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks @dstenb. I have a mix of comments on this one: some minor nits & some more architectural questions which I'd appreciate your input on. That said I don't want to distract from landing this asap and fixing the correctness issue.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1353
+    // entry value.
+    if (DefinedRegs.count(MI.getOperand(0).getReg()))
+      return false;
----------------
dstenb wrote:
> aprantl wrote:
> > I'm assuming this is IsEntryValueCandidate()? There's no context..
> > So we're saying any DBG_VALUE using a register that has been defined in the entry block is not an entry value candidate?
> > I'm assuming this is IsEntryValueCandidate()? There's no context..
> 
> Sorry about not including the context! I have updated the diff.
> 
> > So we're saying any DBG_VALUE using a register that has been defined in the entry block is not an entry value candidate?
> 
> Yes, it will check any DBG_VALUE in the entry block. I think that "have we seen a preceding DBG_VALUE for this parameter which wasn't a viable candidate?" is a condition that is missing here (before this patch also), and in such cases we should not emit entry values for the parameter, Otherwise, I guess that we might emit incorrect entry values in cases like this:
> 
> ```
> void func(int foo, int bar);
> ```
> 
> ```
> entry:
> DBG_VALUE $rdi, $noreg, "foo"
> DBG_VALUE [some value other than a live-in register], "bar"
> [instructions that do not clobber $rdi]
> DBG_VALUE $rdi, $noreg, "bar"
> ```
> 
> I don't think such cases can happen now due to the `isNotModified()` guard, but as we're moving away from that, it's something to consider.
> 
> As a side note, I think it would be neat if there was some side-table mapping parameters to their corresponding registers, in the same as is done for the call sites with the `callSites` metadata, so that we would not have to find entry values by inspecting the entry block's debug values.
Would the side-table mapping parameters to their corresponding registers have to keep track of movements (say, if parameter 1 was moved from %rdi to %r15)?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1304
   // values that are described by the frame or stack pointer.
   if (!isRegOtherThanSPAndFP(MI.getOperand(0), MI, TRI))
     return false;
----------------
Does this intentionally allow non-register uses (like, `DBG_VALUE <constant>, ...`)?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1380
       if (!DebugEntryVals.count(MI.getDebugVariable()))
         DebugEntryVals[MI.getDebugVariable()] = &MI;
+  }
----------------
Perhaps this is out of scope for the current patch, but: Is it possible to find no candidate entry values in the entry block, but to miss candidates in subsequent blocks? Example:

```
define void @callee(i32 %param1) {
entry:
  ; No use of %param1 in entry block: does this imply no entry value?
  br label %succ
succ:
  ; Last use of %param1. We can assume it is dead after the call.
  call void @use(i32 %param1) 
  br label %succ2
succ2:
  ; No entry value location emitted for %param1?
  ret void
}
```


================
Comment at: llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir:146
+    frame-setup CFI_INSTRUCTION def_cfa $r11, 8
+    $r4 = MOVr killed $r0, 14, $noreg, $noreg
+    DBG_VALUE $r4, $noreg, !33, !DIExpression(), debug-location !35
----------------
Nothing to do with this patch, but: Is it expected that "p1" moves from r0 to r4? I guess I'd expect regalloc to keep "p1" in r0, then materialize "p2" in the ABI-specified register for the second argument (r1?). Might be worth a separate bug.


================
Comment at: llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir:149
+    $r0 = MOVi 205, 14, $noreg, $noreg
+    $r0 = ORRri killed $r0, 43776, 14, $noreg, $noreg
+    DBG_VALUE $r0, $noreg, !34, !DIExpression(), debug-location !35
----------------
It might aid readability to add a comment here explaining that the `movi + orrri` pair materializes `0xabcd` in the callee (& that the adjacent DBG_VALUE for !34 defines "p2").


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

https://reviews.llvm.org/D69889