This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Track multiple registers in DbgEntityHistoryCalculator
ClosedPublic

Authored by dstenb on Mar 28 2019, 8:59 AM.

Details

Summary

When calculating the debug value history, DbgEntityHistoryCalculator
would only keep track of register clobbering for the latest debug value
per inlined entity. This meant that preceding register-described debug
value fragments would live on until the next overlapping debug value,
ignoring any potential clobbering. This patch amends
DbgEntityHistoryCalculator so that it keeps track of all registers that
a inlined entity's currently live debug values are described by.

The DebugInfo/COFF/pieces.ll test case has had to be changed since
previously a register-described fragment would incorrectly outlive its
basic block.

The parent patch is expected to increase the coverage slightly, as it
makes sure that location list entries are inserted after clobbered
fragments, and this patch is expected to decrease it, as it stops
preceding register-described from living longer than they should. All
in all, this patch and the preceding patch has a negligible effect
on the output from `llvm-dwarfdump -statistics' for a clang-3.4 binary
built using the RelWithDebInfo build profile. "Scope bytes covered"
increases by 0.5%, and "variables with location" increases from 2212083
to 2212088, but it should improve the accuracy quite a bit.

This fixes PR40283.

Diff Detail

Event Timeline

dstenb created this revision.Mar 28 2019, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 8:59 AM

I think this looks reasonable. Is there any performance hit?

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
131

///
\p Var

161

Should this be a SmallDenseMap?

dstenb added a comment.Apr 2 2019, 2:02 AM

Is there any performance hit?

See https://reviews.llvm.org/D59941#1451056 for some measurements. The decrease in performance appears to be small.

dstenb updated this revision to Diff 193251.Apr 2 2019, 3:48 AM
dstenb marked an inline comment as done.

Address comments.

dstenb marked 2 inline comments as done.Apr 2 2019, 3:49 AM
dstenb added inline comments.
lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
161

Sorry, yes, this was totally overkill. For example, in a RelWithDebInfo+asan build the maximum size of the map was 11, and in 99.98% of the cases the size of the map was less than or equal 2.

dstenb updated this revision to Diff 193289.Apr 2 2019, 7:33 AM
dstenb marked an inline comment as done.

Rebase after update of D59941.

aprantl added inline comments.Apr 2 2019, 4:36 PM
lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
183

if (!TrackedRegs.count(NewReg))

186

for symmetry, perhaps write this as TrackedRegs.insert({NewReg, true});

190

Is it safe to iterate over the map here, or can this introduce nondeterminism?

dstenb updated this revision to Diff 193463.Apr 3 2019, 3:42 AM
dstenb marked 3 inline comments as done.

Address review comments.

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
186

If it is okay, I'd prefer to keep it as it is, to be consistent with the compound-assignment a few lines above:

if (unsigned Reg = isDescribedByReg(DV))
  TrackedRegs[Reg] |= !Overlaps;
190

Yes, the iteration order does not matter here. The only effects (in non-crash cases) that each call to dropRegDescribedVar(RegVars, I.first, Var) has is:

  1. Look up VarSet = RegVars[I.first] in the RegVars map.
  2. Remove Var from the VarSet set.
  3. Remove I.first from RegVars if VarSet is empty.
aprantl accepted this revision.Apr 3 2019, 4:04 PM
aprantl added inline comments.
lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
161

So should it be size 2 then? ;-)

This revision is now accepted and ready to land.Apr 3 2019, 4:04 PM
dstenb marked an inline comment as done.Apr 5 2019, 8:52 AM

Thanks for the review! Just so that I don't misunderstand something; is the LGTM specifically for the changes in this patch, or does that imply the entire patch series?

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
161

Right! I'll change that.

If there are remaining patches that I haven't accepted yet, please ping them.

dstenb added a comment.Apr 8 2019, 9:11 AM

If there are remaining patches that I haven't accepted yet, please ping them.

In addition to D59941, this patch is dependent on four preceding refactoring commits (which are listed in the revision stack). I pinged / added reviewers to them now!

This revision was automatically updated to reflect the committed changes.