[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#697456, @JDevlieghere wrote:

> - Feedback from Teresa and Mehdi
>
>   Regarding Peter's comment: While I like the idea of the `legalToInternalize`, I'm not convinced that it's really better than the callback. Most of the code in `InternalizePass` is concerned with deciding which GVs are legal to be internalized. Performing the actual internalization is only a small part, so why not keep a unified interface like it is today? Either way you'll need info from the linker about what symbols to internalize, and the callback nicely combines (1) indicating whether it is necessary and (2) performing it.


I remembered the other reason I wanted the internalizer to do this - if we have a llvm.used, then likely there is a GV in its aggregate that shouldn't be internalized. The internalizer will handle this appropriately. It also can't be (efficiently) encapsulated in a per-GV "legalToInternalize" helper.

I was thinking more last night about just having the clients call internalizeModule() after module linking (directly, not from a callback) , but the issue becomes that the symbols that were previously in the combined module shouldn't be internalized. So you either need a mechanism for extracting the final value of ValuesToLink from the module linker, and only internalize those, or you need to build a set of defined values in the combined module before calling linkInModule, and use it to disallow internalization of those symbols. I think the callback approach is better, since the module linker has the best knowledge of *which* GVs should be candidates for internalization (aside from the legality of internalization, which is what is being fixed by using internalizeModule).

> PS: I'm working on a test case.

Please include one that has a GV listed in a llvm.used or llvm.compiler.used, and make sure it isn't internalized.

Will take a look at the new version of the patch in a little bit. Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D30738