Page MenuHomePhabricator

[PGO] Move __profc_ and __profvp_ from their own comdat groups to __profd_'s comdat group
ClosedPublic

Authored by MaskRay on Jul 27 2020, 9:21 PM.

Details

Summary

D68041 placed __profc_, __profd_ and (if exists) __profvp_ in different comdat groups.
There are some issues:

  • Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.
  • __profc_, __profd_ and (if exists) __profvp_ should be retained or discarded. Placing them into separate comdat groups is conceptually inferior.
  • If the prevailing group does not include __profvp_ (value profiling not used) but a non-prevailing group from another translation unit has __profvp_ (the function is inlined into another and triggers value profiling), there will be a stray __profvp_ if --gc-sections is not enabled. This has been fixed by 3d6f53018f845e893ad34f64ff2851a2e5c3ba1d.

Actually, we can reuse an existing symbol (we choose __profd_) as the group signature to avoid a string in the string table (the sole reason
that D68041 could improve code size is that __profv_ was an otherwise unused symbol
which wasted string table space).

Diff Detail

Event Timeline

MaskRay created this revision.Jul 27 2020, 9:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 9:21 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2020, 10:04 PM
Harbormaster failed remote builds in B65952: Diff 281103!
MaskRay requested review of this revision.Jul 27 2020, 10:08 PM
MaskRay edited the summary of this revision. (Show Details)Jul 27 2020, 10:14 PM

I think it is better to revert https://reviews.llvm.org/D68041 first with reasons listed, and then follow up with an improvement patch to reduce strtab usage.

I think it is better to revert https://reviews.llvm.org/D68041 first with reasons listed, and then follow up with an improvement patch to reduce strtab usage.

Sent D84766 to revert D68041.

@davidxl I think we probably should just push this instead of going through an intermediate D84766, which might cause a temporary size regression. D84723 is a strict size improvement.

I'll wait two or 3 days to make sure the PGO fix 3d6f53018f845e893ad34f64ff2851a2e5c3ba1d is stable.

It is ok to push this patch, but please update the summary of this patch from https://reviews.llvm.org/D84766 plus the additional improvement bits. Also document the background history a little.

MaskRay edited the summary of this revision. (Show Details)Aug 3 2020, 12:09 PM
MaskRay edited the summary of this revision. (Show Details)
davidxl accepted this revision.Aug 3 2020, 4:45 PM

LGTM ( how much size savings can we expect?)

This revision is now accepted and ready to land.Aug 3 2020, 4:45 PM

Thanks! I performed a build with such a configure line: cmake -Hllvm -B/tmp/out/pgo2 -DCMAKE_BUILD_TYPE=Release ${LLVM_COMMON} -DLLVM_BUILD_INSTRUMENTED=IR -DLLVM_PROFILE_DATA_DIR=/tmp/out/profile After ninja clang lld, of the 2258 object files.

The total size is currently 502034960 bytes. With this patch, 491007080 bytes, 2.2% decrease.

This revision was landed with ongoing or failed builds.Aug 3 2020, 8:40 PM
This revision was automatically updated to reflect the committed changes.