[PATCH] D30738: Don't internalize llvm GV's with InternalizeLinkedSymbols

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere updated this revision to Diff 91218.
JDevlieghere added a comment.

Thanks for the feedback!

In https://reviews.llvm.org/D30738#696758, @tejohnson wrote:

> I'm missing something - if the symbols are already local, why is internalization needed? A test case would help.


I have a test case on my work computer, I'll add it tomorrow.

> But this approach looks much better, thanks!
> 
>> @mehdi_amini: Is this what you had in mind?
>> 
>> I've run the tests and this implementation doesn't break any existing tests regarding linking with the internalize flag.
> 
> There's a user in clang (under OPT_mlink_cuda_bitcode), I would think it would lose the internalization without providing a callback. Maybe there is no test for this case?

That's my bad, I only ran the LLVM tests. I've removed the flag and updated the code in clang (https://reviews.llvm.org/D30792), which is now also passing all tests. I made sure there was an actual test for this by having the callback always return false, which caused some to fail.


Repository:
  rL LLVM

https://reviews.llvm.org/D30738

Files:
  include/llvm/Linker/Linker.h
  lib/Linker/LinkModules.cpp
  tools/llvm-link/llvm-link.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30738.91218.patch
Type: text/x-patch
Size: 7005 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170309/5e38bc89/attachment.bin>