Page MenuHomePhabricator

[ThinLTO] Don't internalize during promotion
ClosedPublic

Authored by evgeny777 on Thu, Oct 17, 6:12 AM.

Details

Summary

This can raise problems when internalised symbol is in comdat, while another member of comdat is exported. Such case is handled by internalize pass which is invoked later.

Diff Detail

Event Timeline

evgeny777 created this revision.Thu, Oct 17, 6:12 AM

I think this change is generally good but sorry I am not quite familiar with the semantics of comdat. Can you explain in more detail why that can be a problem to internalize that in promote?

From https://llvm.org/docs/LangRef.html#comdats :

Comdats have a name which represents the COMDAT key. 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

To enforce this LLVM internalize pass doesn't internalize entire COMDAT group if any of its members should be preserved (see shouldPreserveGV). At the moment we can internalize global
in llvm::thinLTOResolvePrevailingInModule without looking at other COMDAT group members and break this guarantee

From https://llvm.org/docs/LangRef.html#comdats :

Comdats have a name which represents the COMDAT key. 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

To enforce this LLVM internalize pass doesn't internalize entire COMDAT group if any of its members should be preserved (see shouldPreserveGV). At the moment we can internalize global
in llvm::thinLTOResolvePrevailingInModule without looking at other COMDAT group members and break this guarantee

Can you add a testcase which represents the situation?

evgeny777 updated this revision to Diff 225847.Mon, Oct 21, 3:12 AM

Added test case

steven_wu accepted this revision.Mon, Oct 21, 10:26 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Mon, Oct 21, 10:26 AM

@tejohnson Teresa, are you ok with it?

@tejohnson Teresa, are you ok with it?

Yep, just had a chance to look. Suggest adding a comment above the new check that internalization is done separately since the internalization code contains the necessary correctness checks.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 22, 2:30 AM