This is an archive of the discontinued LLVM Phabricator instance.

Do not emit thunks with available_externally linkage in comdats
ClosedPublic

Authored by dschuff on May 7 2015, 3:01 PM.

Details

Summary

Functions with available_externally linkage will not be emitted to object
files (they will just be undefined symbols), so it does not make sense to
put them in comdats.

Diff Detail

Event Timeline

dschuff updated this revision to Diff 25246.May 7 2015, 3:01 PM
dschuff retitled this revision from to Do not emit thunks with available_externally linkage in comdats.
dschuff updated this object.
dschuff added reviewers: rnk, rafael.
dschuff added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.May 7 2015, 4:08 PM

This is correct, but we have lots of instances that should use the same logic:
$ git grep supportsCOMDAT.*isWeakForLinker
lib/CodeGen/CGDecl.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/CGVTT.cpp: if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
lib/CodeGen/CGVTables.cpp: if (CGM.supportsCOMDAT() && Fn->isWeakForLinker())
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker() &&
lib/CodeGen/CodeGenModule.cpp: if (supportsCOMDAT() && GV->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
lib/CodeGen/ItaniumCXXABI.cpp: } else if (CGM.supportsCOMDAT() && guard->isWeakForLinker()) {
lib/CodeGen/ItaniumCXXABI.cpp: if (CGM.supportsCOMDAT() && GV->isWeakForLinker())

How about we add a maybeSetTrivialComdat overload that takes a GlobalObject or something? I'm not asking you to do the migration if you don't want to, but just to add the central place that we should be calling to handle the case where we don't have an easily available Decl with GVA linkage.

Yeah that was actually going to be my followup question, how many other places need this. My concern about just grepping and changing was just the issue I encountered here, where the linkage type was actually not finally set at the point where the comdat was added (hence moving it from generateThunk to emitThunk).

Also it looks like many of those cases can actually use the existing maybeAddTrivialComdat because they have Decls available. Anyway here's another patch with a maybeSetTrivialComdat overload.
Do you have an opinion about whether it should take a GlobalValue or just a GlobalObject? I'm a little bit unclear about linkage and comdats for aliases.

dschuff updated this revision to Diff 25338.May 8 2015, 9:13 AM
dschuff edited edge metadata.
  • use a maybeSetTrivialComdat
dschuff updated this revision to Diff 25342.May 8 2015, 9:40 AM

Actually the other places that needed fixing were all quite straightforward,
so just fix them too. Also make maybeSetTrivialComdat take a GO instead of a GV.

We could maybe even just replace the maybeSetTrivialComdat that takes the Decl
too, but that's probably another CL.

rnk accepted this revision.May 8 2015, 9:46 AM
rnk edited edge metadata.

Thanks! lgtm

lib/CodeGen/CodeGenModule.cpp
1992

indentation seems off

This revision is now accepted and ready to land.May 8 2015, 9:46 AM
dschuff updated this revision to Diff 25343.May 8 2015, 9:48 AM
dschuff edited edge metadata.
  • fix indentation
This revision was automatically updated to reflect the committed changes.