This is an archive of the discontinued LLVM Phabricator instance.

Internalize: internalize comdat members as a group, and drop comdat on such members.
ClosedPublic

Authored by pcc on Jun 23 2015, 5:39 PM.

Details

Summary

Internalizing an individual comdat group member without also internalizing
the other members of the comdat can break comdat semantics. For example,
if a module contains a reference to an internalized comdat member, and the
linker chooses a comdat group from a different object file, this will break
the reference to the internalized member.

This change causes the internalizer to only internalize comdat members if all
other members of the comdat are not externally visible. Once a comdat group
has been fully internalized, there is no need to apply comdat rules to its
members; later optimization passes (e.g. globaldce) can legally drop individual
members of the comdat. So we drop the comdat attribute from all comdat members.

Diff Detail

Event Timeline

pcc updated this revision to Diff 28312.Jun 23 2015, 5:39 PM
pcc retitled this revision from to Internalize: internalize comdat members as a group, and drop comdat on such members..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: majnemer.
pcc added a subscriber: Unknown Object (MLST).
rafael added inline comments.
lib/Transforms/IPO/Internalize.cpp
256–262

Any reason to drop the tracking of Changed? I don't think it makes a difference given how this pass is used, but we should at least comment on why it is not done.

pcc updated this revision to Diff 29513.Jul 11 2015, 1:54 PM
  • Address comment
lib/Transforms/IPO/Internalize.cpp
256–262

Added a comment with an explanation.

rafael accepted this revision.Jul 15 2015, 10:28 PM
rafael added a reviewer: rafael.

LGTM

lib/Transforms/IPO/Internalize.cpp
187

An alias is in the comdat of the aliasee, so I don't think you have to check them.

This revision is now accepted and ready to land.Jul 15 2015, 10:28 PM
pcc added inline comments.Jul 15 2015, 11:10 PM
lib/Transforms/IPO/Internalize.cpp
187

Suppose we have

$c = comdat selectany
@a = external global i32 0, comdat($c)
@b = alias i32* @a

and there exists an external reference to b. In that case we need to expose the c comdat.

$c = comdat selectany
@a = external global i32 0, comdat($c)
@b = alias i32* @a

This is double invalid:

Declaration may not be in a Comdat!
i32* @a
Alias must point to a definition
i32* @b

There are talks of relaxing the second one, but the first will probably stay.

Cheers,
Rafael

pcc added a comment.Jul 16 2015, 9:27 AM

Sorry, I meant this:

$c = comdat any                                                                                                                                                               
@a = global i32 0, comdat($c)                                                                                                                                              
@b = alias i32* @a
This revision was automatically updated to reflect the committed changes.