Page MenuHomePhabricator

[InstrProf] Use separate comdat group for data and counters
ClosedPublic

Authored by rnk on Feb 27 2019, 1:41 PM.

Details

Summary

I hadn't realized that instrumentation runs before inlining, so we can't
use the function as the comdat group. Doing so can create relocations
against discarded sections when references to discarded __profc_
variables are inlined into functions outside the function's comdat
group.

In the future, perhaps we should consider standardizing the comdat group
names that ELF and COFF use. It will save object file size, since
__profv_$sym won't appear in the symbol table again.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Feb 27 2019, 1:41 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptFeb 27 2019, 1:41 PM
Herald added subscribers: Restricted Project, cfe-commits, hiraditya, eraman. · View Herald Transcript
vsk added a comment.Feb 27 2019, 2:38 PM

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

Also, why is the selection kind here 'any' instead of 'exactmatch'? If the counter array sizes legitimately can vary (why?), then wouldn't we want 'largestsize'?

xur added inline comments.Feb 27 2019, 2:50 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
756 ↗(On Diff #188612)

So with the patch, COFF and ELF have the same way of naming. Can we simplify the code by hosting the common code?

rnk marked 2 inline comments as done.Feb 27 2019, 3:04 PM
In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

Also, why is the selection kind here 'any' instead of 'exactmatch'? If the counter array sizes legitimately can vary (why?), then wouldn't we want 'largestsize'?

I think if the data size varies, then we are out of luck. Every TU will produce the same data if the profile intrinsics are inserted at the same point in the pipeline. Also, both largestsize and exactmatch are COFF-only.

For code coverage, the instrprof.increment calls are inserted very early, and it is source-directed, so we should be pretty confident that all profile data for a given inline function is identical across the whole program. Of course, different versions of clang may insert counters differently, so if you link together object files produced by two versions of clang, you run the risk of having corrupt profile data.

For PGO, I have less confidence that that is true, but I believe InstrProf is carefully placed in the pipeline (before the main inliner pass, preinlining only) at such a point to reduce the likelihood that this happens. Of course, PGO already locks in the compiler version of the whole program. You can't optimize with instrumentation inserted by the last release of clang.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
756 ↗(On Diff #188612)

It's not identical yet, though, but maybe it should be. For COFF, the comdat is __profc_$sym, but for ELF, it is __profv_$sym. However, they are very similar, and I can factor out the common code now without changing ELF to match COFF.

rnk updated this revision to Diff 188635.Feb 27 2019, 3:06 PM
rnk marked an inline comment as done.
  • share code
rnk added a comment.Feb 27 2019, 3:17 PM

Oops, forgot to respond to this...

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

I agree, that wording seems very confusing. I probably even wrote it. I'll try editing it and send you the result later.

These words are trying to say that, some number of global objects can use a comdat key. If the object file they are contained in has the prevailing definition of that comdat key, then they will be included, and they will be discarded if not. The prevailing object is typically just the first object file that uses the comdat.

So, multiple objects from *one TU* can use the key and end up in the final binary, but objects from different TUs will never end up mixed together in the final binary.

xur accepted this revision.Feb 27 2019, 3:17 PM

lgtm.

BTW, I'm in the process of committing D54175. After that patch, PGO instrumentation can be called after the main inlining. I don't think it will conflict anything in this patch

This revision is now accepted and ready to land.Feb 27 2019, 3:17 PM
rnk added a comment.Feb 27 2019, 3:24 PM
In D58737#1412847, @xur wrote:

lgtm.

BTW, I'm in the process of committing D54175. After that patch, PGO instrumentation can be called after the main inlining. I don't think it will conflict anything in this patch

I see, I guess I'll update the comment to say "may run before the inliner".

This revision was automatically updated to reflect the committed changes.
vsk added a comment.Feb 28 2019, 10:38 AM
In D58737#1412846, @rnk wrote:

Oops, forgot to respond to this...

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

These words are trying to say that, some number of global objects can use a comdat key. If the object file they are contained in has the prevailing definition of that comdat key, then they will be included, and they will be discarded if not. The prevailing object is typically just the first object file that uses the comdat.

So, multiple objects from *one TU* can use the key and end up in the final binary, but objects from different TUs will never end up mixed together in the final binary.

Ah, got it, thanks :)