Page MenuHomePhabricator

[DebugInfo][NFC] Early-exit when analysing for single-location variables
ClosedPublic

Authored by jmorse on Apr 7 2020, 4:36 AM.

Details

Summary

This is a performance patch that hoists two conditions in DwarfDebug's validThroughout to avoid a linear-scan of all instructions in a block. We now exit early if validThrougout will never return true for the variable location. No test as this is a performance change, but:

  • It halves the time in DWARF emission for several code samples I'm working on, which have large volumes of inlining to deal with,
  • A clang-10 stage2 build produces an identical binary with/without this patch.

The first added clause filters for the two circumstances where validThroughout will return true. The second added clause should be identical to the one that's deleted from after the linear-scan.

Diff Detail

Event Timeline

jmorse created this revision.Apr 7 2020, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 4:36 AM
dstenb added a comment.Apr 7 2020, 6:20 AM

Adding a nit. I'll try to take a closer look at the functionality later today.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1483

Nit: neither *is* ?

dstenb accepted this revision.Apr 7 2020, 9:41 AM

Two minor nits, but otherwise this looks good to me. Thanks!

(It would be interesting to revisit this "Single, constant DBG_VALUEs" behavior sometime in the future, but I don't expect you do that just because you improved the performance of the existing behavior.)

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1485

Nit: It might be preferred to use !RangeEnd here since the rest of the code in this function does nullptr checks like that.

This revision is now accepted and ready to land.Apr 7 2020, 9:41 AM
aprantl added inline comments.Apr 7 2020, 10:31 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1485

You probably mean RangeEnd?
I also think that is better.

1498

like here

dstenb added inline comments.Apr 7 2020, 12:55 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1485

Oops, yes!

This revision was automatically updated to reflect the committed changes.
jmorse added a comment.Apr 8 2020, 5:06 AM

Thanks for the reviews,

David wrote:

(It would be interesting to revisit this "Single, constant DBG_VALUEs" behavior sometime in the future, but I don't expect you do that just because you improved the performance of the existing behavior.)

Weeelllll, I did toy with this a while back, I think the fact that we only emit single locations for "open ended" (i.e. is the last block in the function) locations is a large limitation. I tried inserting another piece of linear-scanning code [0] to test the following:

  • The lexical scope only covers one block (this one),
  • The variable location starts before/at the start of the scope,
  • The variable location ends at/after the end of the scope

Of which the third test is new, and the first two are already tested by validThroughout.

The result was, on a clang-trunk stage2reldeb build on Ubuntu 18.04:

  • a 27% reduction in the size of .debug_loc (450MiB -> 329MiB),
  • Leading to a 6% reduction in binary size overall (2077MiB -> 1954MiB).

Which is quite attractive. Pursuing this involves making an argument that representing this situation with a single-location in DWARF, and I'm not familiar enough with DWARF right now. The hard part however is doing this efficiently, as touching every instruction in a block for every variable doesn't scale. (If anyone picks this up, please let me know, otherwise we'll eventually get around to addressing this).

[0] https://github.com/jmorse/llvm-project/commit/903338d971c9c1c9c46a6e1aebc6f1e1b405d37c