Page MenuHomePhabricator

[ThinLTO] Remove imported available externally defs from comdats.
ClosedPublic

Authored by tejohnson on Jan 12 2016, 11:35 AM.

Details

Summary

Available externally definitions are considered declarations for the
linker and eventually dropped. As such they are not allowed to be
in comdats. Remove any such imported functions from comdats.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 44655.Jan 12 2016, 11:35 AM
tejohnson retitled this revision from to [ThinLTO] Remove imported available externally defs from comdats..
tejohnson updated this object.
tejohnson added a reviewer: rafael.
tejohnson added subscribers: llvm-commits, davidxl.

Ping.

Thanks,
Teresa

mehdi_amini added inline comments.Feb 8 2016, 8:10 AM
lib/Linker/LinkModules.cpp
733 ↗(On Diff #44655)

I don't understand why we need to this?

737 ↗(On Diff #44655)

This comment looks like it is a bug in the IRMover that you're working around here?

tejohnson added inline comments.Feb 8 2016, 8:18 AM
lib/Linker/LinkModules.cpp
733 ↗(On Diff #44655)

Declarations (more generally declarations for the linker, so including available_externally) may not be in a comdat. It is illegal (and checked by Verifier::visitGlobalValue).

737 ↗(On Diff #44655)

No, it is describing the below assert. I.e. the only declarations for the linker that are in a comdat at this point would be imported definitions, which are available externally. I will try to clarify the comment a bit.

tejohnson updated this revision to Diff 47210.Feb 8 2016, 8:28 AM
  • Expand comments to better explain what and what this code is doing.
mehdi_amini requested changes to this revision.Feb 8 2016, 10:01 AM
mehdi_amini added a reviewer: mehdi_amini.

LGTM

lib/Linker/LinkModules.cpp
733 ↗(On Diff #47210)

Ok I didn't know it was illegal :)

This revision now requires changes to proceed.Feb 8 2016, 10:01 AM
mehdi_amini accepted this revision.Feb 8 2016, 10:01 AM
mehdi_amini edited edge metadata.
This revision is now accepted and ready to land.Feb 8 2016, 10:01 AM
This revision was automatically updated to reflect the committed changes.