This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Avoid creating entry values for clobbered registers
ClosedPublic

Authored by dstenb on Nov 6 2019, 5:54 AM.

Details

Summary

Entry values are considered for parameters that have register-described
DBG_VALUEs in the entry block (along with other conditions).

If a parameter's value has been propagated from the caller to the
callee, then the parameter's DBG_VALUE in the entry block may be
described using a register defined by some instruction, and entry values
should not be emitted for the parameter, which can currently occur.
One such case was seen in the attached test case, in which the second
parameter, which is described by a redefinition of the first parameter's
register, would incorrectly get an entry value using the first
parameter's register. This commit intends to solve such cases by keeping
track of register defines, and ignoring DBG_VALUEs in the entry block
that are described by such registers.

In a RelWithDebInfo build of clang-8, the average size of the set was
27, and in a RelWithDebInfo+ASan build it was 30.

Diff Detail

Event Timeline

dstenb created this revision.Nov 6 2019, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 5:54 AM

@dstenb Thanks!

llvm/lib/CodeGen/LiveDebugValues.cpp
1326

Can we put this in a helper?

1331

Maybe all of this can go in a helper as well. I think that should simplify the code here.

dstenb marked 3 inline comments as done.
dstenb added inline comments.
llvm/lib/CodeGen/LiveDebugValues.cpp
1331

Yes, good idea! I put that refactoring in a parent patch.

aprantl added inline comments.Nov 7 2019, 5:41 PM
llvm/lib/CodeGen/LiveDebugValues.cpp
1253

I assume you checked that such a helper doesn't already exist?

1353

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?

dstenb updated this revision to Diff 228417.Nov 8 2019, 4:42 AM
dstenb marked an inline comment as done.

Rebase (IsEntryValueCandidate function moved to a separate function), and include full diff context.

djtodoro added inline comments.Nov 8 2019, 5:00 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
1285

const DefinedRegsSet&?

1374

Perhaps we can add a comment here describing what is the purpose of adding this.

dstenb marked 2 inline comments as done.Nov 8 2019, 5:32 AM
dstenb added inline comments.
llvm/lib/CodeGen/LiveDebugValues.cpp
1253

Yes, I have not been able to find any functions that collects the defs in this way.

1353

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.

vsk added a comment.Nov 8 2019, 12:50 PM

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.

llvm/lib/CodeGen/LiveDebugValues.cpp
1308

Does this intentionally allow non-register uses (like, DBG_VALUE <constant>, ...)?

1353

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)?

1382–1387

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
}
llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir
147

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.

150

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").

vsk added inline comments.Nov 8 2019, 1:34 PM
llvm/lib/CodeGen/LiveDebugValues.cpp
1308

I totally misread the condition, please ignore.

dstenb updated this revision to Diff 228701.Nov 11 2019, 8:05 AM
dstenb marked 9 inline comments as done.

Address review comments.

dstenb added inline comments.Nov 11 2019, 8:07 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
1285

Yes, thanks!

1353

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.

1382–1387

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

llvm/test/DebugInfo/MIR/ARM/dbgcall-site-propagated-value.mir
147

I think it is as expected, since the second parameter's value is passed in r0 in the call to interesting() below here.

vsk accepted this revision.Nov 11 2019, 9:51 AM

Looks good to me.

llvm/lib/CodeGen/LiveDebugValues.cpp
1382–1387

Oh, interesting. Perhaps, as a follow-up, we could try asserting that we get that behavior from ISel & onwards ('if an MI outside of First_MBB is an entry value candidate, & its DIVar is not in the entry vals map, panic').

This revision is now accepted and ready to land.Nov 11 2019, 9:51 AM

Any more comments on this, @djtodoro?

djtodoro accepted this revision.Nov 13 2019, 12:43 AM

LGTM as well. Thanks @dstenb!