Page MenuHomePhabricator

[PGO] Use linkonce_odr linkage for __profd_ variables in comdat groups
ClosedPublic

Authored by rnk on Sep 13 2019, 3:42 PM.

Details

Summary

This fixes relocations against __profd_ symbols in discarded sections,
which is PR41380.

This is a sketch of the way things are supposed to work:

  1. IR is instrumented relatively early in pass pipeline with llvm.instrprof.increment intrinsic calls
  2. Inlining and optimization occurs
  3. Intrinsics are lowered and __prof[cdv]_ globals are created

For C++ inline functions of all kinds (linkonce_odr &
available_externally mainly), instr profiling wants to deduplicate these
profc_ and profd_ globals. Otherwise the binary would be quite
large. For a popular inline function 'foo', it is expected that
__profc_foo will be referenced wherever foo is inlined, even if a
standalone body of 'foo' is omitted.

I made profd_ and profc_ comdat in r355044, but I chose to make
profd_ internal. At the time, I was only dealing with coverage, and in
that case, none of the instrumentation needs to reference
profd_.
However, if you use PGO, then instrumentation passes add calls to
llvm_profile_instrument_range which reference profd_ globals. The
solution is to make these globals externally visible by using
linkonce_odr linkage for data as was done for counters.

This is safe because PGO adds a CFG hash to the names of the data and
counter globals, so if different TUs have different globals, they will
get different data and counter arrays.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Sep 13 2019, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 3:42 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
xur accepted this revision.Sep 13 2019, 4:04 PM

LGTM. Thanks for the fix!

I wasn't sure why profd was set to internal when looking at PR41380 (as for ELF, all are linkonce_oda). But your description explained that.

BTW, the intrinsic lowering happens before inline (I suppose you are talking the main IPA inline).

This revision is now accepted and ready to land.Sep 13 2019, 4:04 PM
rnk added a comment.Sep 13 2019, 4:17 PM
In D67579#1670170, @xur wrote:

LGTM. Thanks for the fix!

I wasn't sure why profd was set to internal when looking at PR41380 (as for ELF, all are linkonce_oda). But your description explained that.

BTW, the intrinsic lowering happens before inline (I suppose you are talking the main IPA inline).

I see. It's unfortunately very hard to figure out how the pipeline works, only the new pass manager seems to support the -fdebug-pass-manager flag.

I guess they key thing is that the instrumentation happens in the frontend or in IR before inlining, and after inlining, functions can reference many different __profc_ variables, some of which can be deduplicated.

xur added a comment.Sep 13 2019, 4:25 PM

For the legacy pass manager, use option "-mllvm -debug-pass=Structure", or in the source, we add the lowering pass right after instrumentation pass.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Sep 17 2019, 11:03 AM

Hm, this broke check-asan on Windows, which exercises some code coverage features:
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/51563

More debugging is required.

rnk added a comment.Sep 17 2019, 3:13 PM

From what I understand, the comdat group isn't necessary, so I disabled it on COFF in r372182. Can we simplify ELF to match?