[PATCH] D131043: [InstrProf] Don't use comdat for private global variables on COFF

[Reid Kleckner via Phabricator via llvm-commits]

rnk requested changes to this revision.
rnk added a comment.
This revision now requires changes to proceed.

I don't think this current revision will work, I don't imagine you would be able to build Chrome for Windows with coverage with this change, or if you did, the binary and object file sizes would be much larger.



================
Comment at: llvm/test/Instrumentation/InstrProfiling/comdat.ll:33
 ; COFF0: @__profc_foo_inline = linkonce_odr hidden global {{.*}}, section ".lprfc$M", comdat, align 8
-; COFF0: @__profd_foo_inline = private global {{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8
 ; COFF1: @__profc_foo_inline = linkonce_odr hidden global {{.*}}, section ".lprfc$M", comdat, align 8
----------------
This seems like an interesting behavior change. Why do we want this? The intention of the current structure is that the profd data reuses the profc comdat, so we don't need to include profd in the symbol table. I believe I did this to reduce object file size and symbol table size. If we don't use a comdat here, then we will end up with extra duplicate profd data that we don't want.


================
Comment at: llvm/test/Instrumentation/InstrProfiling/comdat.ll:60
+; COFF1: @__profd_foo_private_profn = private global{{.*}}, section ".lprfd$M", align 8
+define void @foo_private_profn() comdat {
+  call void @llvm.instrprof.increment(ptr @__profn_foo_private_profn, i64 0, i32 1, i32 0)
----------------
It sounds like the real corner case here is doing coverage for functions with private linkage. To handle that case, I think we should make the __profc data internal linkage instead of private. Internal linkage will produce a symbol table entry which can be used with comdats.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131043