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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D30738#696733, @JDevlieghere wrote:

> @tejohnson The symbols are local, so it doesn't cause an issue with regular linking. It only happens when we throw all the modules together.


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

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?



================
Comment at: lib/Linker/LinkModules.cpp:110
+               std::function<void(Module &, StringSet<>)> InternalizeCallback =
+                   std::function<void(Module &, StringSet<>)>())
+      : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags),
----------------
Think the default param should be "{}" for consistency with linkInModules and with check below for a non-null InternalizeCallback.


================
Comment at: lib/Linker/LinkModules.cpp:554
 
-  for (auto &P : Internalize) {
-    GlobalValue *GV = DstM.getNamedValue(P.first());
-    GV->setLinkage(GlobalValue::InternalLinkage);
-  }
+  if (shouldInternalizeLinkedSymbols() && InternalizeCallback)
+    InternalizeCallback(DstM, Internalize);
----------------
Presumably the flag can go away, it is subsumed by the presence of the callback. There are a couple of uses in clang itself that will need to change to pass in a callback.


================
Comment at: tools/llvm-link/llvm-link.cpp:44
 
-static cl::list<std::string>
-InputFilenames(cl::Positional, cl::OneOrMore,
-               cl::desc("<input bitcode files>"));
+static cl::list<std::string> InputFilenames(cl::Positional, cl::OneOrMore,
+                                            cl::desc("<input bitcode files>"));
----------------
Don't clang format the whole file with your patch. You can commit a separate patch with just the clang formating, or just clang format your changes.


Repository:
  rL LLVM

https://reviews.llvm.org/D30738