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

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1353
+    // entry value.
+    if (DefinedRegs.count(MI.getOperand(0).getReg()))
+      return false;
----------------
vsk wrote:
> 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)?
This is just an idea that I threw out without giving it a lot of thought, but I imagined that the side table would only specify in which register(s) each parameter reside in at function entry, and we'd then leave it up to LiveDebugValues to track the register movements.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1281
+bool LiveDebugValues::isEntryValueCandidate(const MachineInstr &MI,
+                                            DefinedRegsSet &DefinedRegs) const {
   if (!MI.isDebugValue())
----------------
djtodoro wrote:
> `const DefinedRegsSet&`?
Yes, thanks!


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1380
       if (!DebugEntryVals.count(MI.getDebugVariable()))
         DebugEntryVals[MI.getDebugVariable()] = &MI;
+  }
----------------
vsk wrote:
> 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
> }
> ```
Testing this throughout the day, I was not able to get such a case when compiling Csmith programs at least. SelectionDAGBuilder puts some effort into placing the parameters' first DBG_VALUEs at the start of the entry block [0], so perhaps we're safe from that (or perhaps it's at least unlikely)? Other passes, e.g. PrologEpilogInserter also try to keep parameter DBG_VALUEs at the start of the entry block [1].

[0] https://github.com/llvm/llvm-project/blob/2028ae975c6aa65df3d89c10c95bf9b7baa5e0ab/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L5279
[1]https://github.com/llvm/llvm-project/blob/2028ae975c6aa65df3d89c10c95bf9b7baa5e0ab/llvm/lib/CodeGen/PrologEpilogInserter.cpp#L177


================
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
----------------
vsk wrote:
> 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.
I think it is as expected, since the second parameter's value is passed in r0 in the call to `interesting()` below here.


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

https://reviews.llvm.org/D69889