[PATCH] D69889: [DebugInfo] Avoid creating entry values for clobbered registers
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 8 12:53:01 PST 2019
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
More information about the llvm-commits
mailing list