Page MenuHomePhabricator

[PGO] Enable profile name compression
ClosedPublic

Authored by davidxl on Jan 20 2016, 5:43 PM.

Details

Summary

As discussed in RFC, this patch does the following

  1. Changes the raw format to support compression
  2. Keeps the indexed format version
  3. change the coverage map format but in backward compatible way.

There is no user level tool interface/usability change.

The impact of this patch -- for clang build with -fprofile-instr-generate -fcoverage-mapping and GC ,

  1. Instrumented clang size is reduced from 273M to 201M -- a 72 MB reduction (in text and data)
  2. Total input object size to linker is reduced from 1187 MB to 913 MB -- a 274 MB reduction
  3. Raw profile size is reduced from 147 MB to 75 MB

This patch only include the LLVM change.

  1. the main compile-rt change just need to update InstrProfData.inc
  2. there are about 4 test cases that need to update raw profile data (binary)
  3. there are a few expected .ll file change
  1. For platforms (not linux, freebsd, nor mach-o) that do not support finding section start/end with special symbols, compiler runtime change is needed to add a name section registration -- It will be in a different patch.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 45467.Jan 20 2016, 5:43 PM
davidxl retitled this revision from to [PGO] Enable profile name compression.
davidxl updated this object.
davidxl added reviewers: vsk, bogner, silvas.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Jan 26 2016, 3:37 PM

Thanks, this is awesome!

  1. I've noticed that some fields (e.g NameRef) are typed as uint64_t and as IntPtrT somewhat interchangeably. Wdyt of adopting a convention where IntPtrT is reserved for values which have already been byte-swapped?
  1. There are some binary tests in test/tools/llvm-cov/Inputs/ for version 1 of the coverage mapping format. Could you add similar tests for version 2?
include/llvm/ProfileData/InstrProfData.inc
164 ↗(On Diff #45467)

I think an updated to CoverageMappingFormat.rst belongs with this commit. This should be a fairly small change (mostly lines removed).

lib/Transforms/Instrumentation/InstrProfiling.cpp
375 ↗(On Diff #45467)

If we don't create NamesVar is there no name section, and if so, is that ever a problem?

385 ↗(On Diff #45467)

For consistency, could you add a getter for "__llvm_prf_nms" to InstrProf.h?

davidxl updated this revision to Diff 46076.Jan 26 2016, 4:30 PM
davidxl edited edge metadata.

Update the patch with a new internal option for testing.

NameRef in V1 is IntPtr type (with different size between 32 and 64bit target), but in V2 it becomes uint64_t consistently. Yes will add coverage V2 testing (better as a follow up patch I think).

include/llvm/ProfileData/InstrProfData.inc
164 ↗(On Diff #46076)

good point. Will update.

lib/Transforms/Instrumentation/InstrProfiling.cpp
379 ↗(On Diff #46076)

That means this module won't have data and counter (i.e, no emitted functions) sections either -- so there is no problem.

389 ↗(On Diff #46076)

Ok to add ( I did not add it because this variable now strictly has internal linkage which runtime does not care about).

davidxl updated this revision to Diff 46091.Jan 26 2016, 7:39 PM

Addressed vsk's review feedbacks.

davidxl updated this revision to Diff 46110.Jan 27 2016, 12:14 AM

Update the patch to make it fully function complete (other than test case update).

  • added support for name runtime registration needed on platforms other than darwin, linux, and freebsd.

The runtime part of the patch is: http://reviews.llvm.org/D16624

davidxl updated this revision to Diff 46346.Jan 28 2016, 8:50 PM

Update the patch with test case changes.

This is now the complete LLVM patch.

vsk added a comment.Feb 3 2016, 3:43 PM

I think it'd be fair to add the V2 binary tests after committing, once we're sure the bots are stable. A few more comments --

lib/Transforms/Instrumentation/InstrProfiling.cpp
32 ↗(On Diff #46346)

IMO this works fine as a non-hidden option, but I'll leave it up to you.

296 ↗(On Diff #46346)

This change seems inconsistent with the comment above it -- why start using the counter var prefix for COFF comdats?

372 ↗(On Diff #46346)

Why is this step required, and how do we know this is the right linkage to reset to? E.g in swift, these name pointers start life with LinkOnceAny linkage.

374 ↗(On Diff #46346)

Where would these names be stripped out? IIUC this vector is in place so we can call collectPGOFuncNameStrings in emitNameData.

davidxl added inline comments.Feb 3 2016, 10:16 PM
lib/Transforms/Instrumentation/InstrProfiling.cpp
32 ↗(On Diff #46346)

ok will make it non hidden.

296 ↗(On Diff #46346)

Will fix the comment. The name var comdat no long exists.

372 ↗(On Diff #46346)

It is set to privateLinkage so that the name vars can be garbage collected by the compiler. Note that the individual name variable (per function) is created by instrumenter to pass the linkage that can be passed on to the counter and data variable. Once that is done, they are no longer needed. Will update the comment.

374 ↗(On Diff #46346)

This is certainly an out of place comment. Will remove.

davidxl updated this revision to Diff 46967.Feb 4 2016, 3:22 PM

Addressed vsk's review feedbacks.

vsk added a comment.Feb 4 2016, 4:46 PM

Lgtm.

lib/Transforms/Instrumentation/InstrProfiling.cpp
296 ↗(On Diff #46346)

D'oh. Right.

This revision was automatically updated to reflect the committed changes.