This is an archive of the discontinued LLVM Phabricator instance.

[InstrProf] Implement static profdata registration
ClosedPublic

Authored by rnk on Feb 7 2019, 3:16 PM.

Details

Summary

The motivating use case is eliminating duplicate profile data registered
for the same inline function in two object files. Before this change,
users would observe multiple symbol definition errors with VC link, but
links with LLD would succeed.

Users (Mozilla) have reported that PGO works well with clang-cl and LLD,
but when using LLD without this static registration, we would get into a
"relocation against a discarded section" situation. I'm not sure what
happens in that situation, but I suspect that duplicate, unused profile
information was retained. If so, this change will reduce the size of
such binaries with LLD.

Now, Windows uses static registration and is in line with all the other
platforms.

Event Timeline

rnk created this revision.Feb 7 2019, 3:16 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 7 2019, 3:16 PM
Herald added subscribers: Restricted Project, hiraditya, fedor.sergeev and 3 others. · View Herald Transcript
davidxl added inline comments.Feb 7 2019, 3:34 PM
compiler-rt/lib/profile/InstrProfilingPlatformOther.c
64

register function won't be called with static registration. Why is this change needed?

rnk updated this revision to Diff 185881.Feb 7 2019, 3:43 PM
  • Rework InstrProfData.inc to avoid warnings about '$' in identifiers
rnk marked an inline comment as done.Feb 7 2019, 3:45 PM
rnk added inline comments.
compiler-rt/lib/profile/InstrProfilingPlatformOther.c
64

This change isn't needed anymore. I suppose Solaris is the only supported platform that uses this now, and perhaps the risk that the linker lays out the profile data discontiguously is negligible.

rnk updated this revision to Diff 185882.Feb 7 2019, 3:46 PM
  • Revert unnecessary PlatformOther changes
wmi edited reviewers, added: xur; removed: wmi.Feb 7 2019, 3:48 PM
wmi added a subscriber: wmi.
davidxl added inline comments.Feb 7 2019, 4:06 PM
compiler-rt/lib/profile/InstrProfilingValue.c
34

What is this change for?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
682

Why is const removed?

758

can this code be moved closer to where COFF linkage handling above (i.e. resetting to internal)?

901

insertin --> inserting

rnk marked 5 inline comments as done.Feb 7 2019, 4:14 PM
rnk added inline comments.
compiler-rt/lib/profile/InstrProfilingValue.c
34

When I compiled my original change on Linux, I encountered warnings about this code:

#define INSTR_PROF_DATA_COFF .lprfd$M

Clang warns that dollars in identifiers are considered to be an extension. To address that, I quoted the COFF strings, and eliminated the *_STR macros.

The only use of the section names that needs to not be quoted is the use in InstrProfilingPlatformLinux.c, which I modified to use INSTR_PROF_INSTR_PROF_DATA_COMMON to build the start / stop section bounds symbols. This code is already platform specific, so it seemed OK to use the underlying *_COMMON macros.

If you like, I can separate that change out. Would you like me to upload it as a separate review?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
682

Oops, not intended.

758

I'll try something. I think it might be best to re-inline the code that computes the comdat to use, because the linkage used is very related to how that is done.

rnk updated this revision to Diff 185887.Feb 7 2019, 4:33 PM
rnk marked an inline comment as done.
  • refactor linkage and comdat calculation
davidxl accepted this revision.Feb 7 2019, 4:45 PM

lgtm.

compiler-rt/lib/profile/InstrProfilingValue.c
34

ok.

This revision is now accepted and ready to land.Feb 7 2019, 4:45 PM
This revision was automatically updated to reflect the committed changes.