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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D30738#697081, @mehdi_amini wrote:

> In https://reviews.llvm.org/D30738#697051, @pcc wrote:
>
> > The fix can be simplified to
> >
> >   if (P.first().startswith("llvm."))
> >     continue;
> >
>
>
> I wouldn't want to see this, this is still duplicating logic. I'd be OK with a helper function`legalToInternalize(GlobalValue &)` that would factor out the logic to decide this.


Works for me, I guess.

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

> In https://reviews.llvm.org/D30738#697051, @pcc wrote:
>
> > The fix can be simplified to
> >
> >   if (P.first().startswith("llvm."))
> >     continue;
> >
> >
> > That is two lines of code. Even if we simplified the internalize pass in the same way, we should not add this much complexity just to avoid duplicating two lines.
>
>
> Sure that is simple from a # lines standpoint, but the internalizer already had all the right logic, so we wouldn't have had this issue if we used internalizeModule to start with. My original suggestion was to move it out of here completely and have the clients invoke internalizeModule directly if they want to internalize, since I'm not sure the module linker should be in the business of doing internalization.


I'm not sure about that either. We may want to do something like what you propose, but it seems orthogonal to fixing the bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D30738