This is an archive of the discontinued LLVM Phabricator instance.

New pass to make internal linkage symbol names unique
ClosedPublic

Authored by tmsriram on Apr 15 2020, 2:19 PM.

Details

Summary

This is the parent pass of D73307.

This adds a new pass to make internal linkage symbol names unique with clang option -funique-internal-linkage-symbols.

Diff Detail

Event Timeline

tmsriram created this revision.Apr 15 2020, 2:19 PM
mtrofin added inline comments.Apr 15 2020, 4:01 PM
llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
44

when are you returning false?

rnk added a comment.Apr 17 2020, 10:28 AM

Code looks good, but I think this needs a test in llvm/test/Transforms. I think it should be registered by the INITIALIZE_PASS macros, so opt -unique-internal-linkage-names should find it and run it...

tmsriram marked an inline comment as done.Apr 17 2020, 12:51 PM
In D78243#1989248, @rnk wrote:

Code looks good, but I think this needs a test in llvm/test/Transforms. I think it should be registered by the INITIALIZE_PASS macros, so opt -unique-internal-linkage-names should find it and run it...

Done.

tmsriram updated this revision to Diff 258403.Apr 17 2020, 12:52 PM

Add test to llvm/test/Transforms

rnk accepted this revision.Apr 17 2020, 1:11 PM

lgtm, plus improvement to test.

llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
3

I suspect Module::getSourceFilename in this example returns "", so you are adding a hash of the empty string. I think it would be better to add source_filename = "foo.cpp" here, and then explicitly check for the hex bits in the names. That ensures that everyone gets the same results everywhere, and will detect changes in the hashing.

This revision is now accepted and ready to land.Apr 17 2020, 1:11 PM
tmsriram marked an inline comment as done.Apr 17 2020, 1:59 PM
tmsriram added inline comments.
llvm/test/Transforms/UniqueInternalLinkageNames/unique_symbol_names.ll
3

Great catch! You are right. I wrongly assumed that the module name would be .ll file name but since I use '<' it is the empty string. I will fix it.

tmsriram updated this revision to Diff 258427.Apr 17 2020, 2:30 PM

Add source_filename to test and make the hash explicit.

MaskRay added inline comments.Apr 17 2020, 4:48 PM
llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
2
tmsriram updated this revision to Diff 258451.Apr 17 2020, 5:26 PM
tmsriram marked an inline comment as done.

Fix file header.

tmsriram updated this revision to Diff 258452.Apr 17 2020, 5:31 PM

Fix header.

You may need an opt -passes='...' test for the new pass manager and implement the new pass.

We are using -fexperimental-new-pass-manager heavily.

llvm/include/llvm/Transforms/Utils/UniqueInternalLinkageNames.h
28

The defaulted default constructor can be deleted.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
31

(Twine('.') + Twine(Str))

You may need an opt -passes='...' test for the new pass manager and implement the new pass.

We are using -fexperimental-new-pass-manager heavily.

I am already testing -fexperimental-new-pass-manager in D73307, need to do it here?

You may need an opt -passes='...' test for the new pass manager and implement the new pass.

We are using -fexperimental-new-pass-manager heavily.

I am already testing -fexperimental-new-pass-manager in D73307, need to do it here?

Probably wouldn't hurt to have both an explicit test for each pass manager.

This revision was automatically updated to reflect the committed changes.
tmsriram marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 3:13 PM

You may need an opt -passes='...' test for the new pass manager and implement the new pass.

We are using -fexperimental-new-pass-manager heavily.

I am already testing -fexperimental-new-pass-manager in D73307, need to do it here?

Probably wouldn't hurt to have both an explicit test for each pass manager.

I saw this after I pushed. I can follow it up with a test.