[PATCH] D71572: [ItaniumCXXABI] Make tls wrappers comdat on Windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2551
+  // actually handle multiple TUs defining the same wrapper.
+  if (CGM.getTriple().isOSWindows() && CGM.supportsCOMDAT() &&
+      Wrapper->isWeakForLinker())
----------------
mstorsjo wrote:
> rnk wrote:
> > IMO we should be doing the same thing on ELF, you can see the code pattern used elsewhere:
> > http://llvm-cs.pcc.me.uk/?q=setComdat
> > 
> > Before comdats were made explicit in the IR and we stopped implicitly making comdat groups for odr things on ELF, these wrappers would've been in a comdat group.
> Ok, so remove the `isOSWindows()` and maybe remove the comment altogether as it isn't windows specific any longer?
In the current tests, for linux there's explicit checks that these aren't marked as comdat, see e.g. https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGenCXX/cxx11-thread-local.cpp#L208. Is there a specific reason for that, or should it be changed?


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

https://reviews.llvm.org/D71572