[PATCH] D58737: [InstrProf] Use separate comdat group for data and counters

Wed Feb 27 15:04:12 PST 2019

rnk marked 2 inline comments as done.
rnk added a comment.

In D58737#1412734 <https://reviews.llvm.org/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.

================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:756
+      // For COFF, the comdat group name must be the name of a symbol in the
+      // group. Use the counter variable name.
+      Cmdt = M->getOrInsertComdat(
----------------
xur wrote:
> So with the patch, COFF and ELF have the same way of naming. Can we simplify the code by hosting the common code?
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58737/new/

https://reviews.llvm.org/D58737