[PATCH] D26880: [ThinLTO] Fix crash: transitively referenced global shouldn't be promoted

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26880#600927, @mehdi_amini wrote:

> In https://reviews.llvm.org/D26880#600909, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D26880#600745, @mehdi_amini wrote:
> >
> > > So my current feeling is:
> > >
> > > - the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.
> >
> >
> > The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.
>
>
> I don't get this part? 
>  I expect the imported to *control* what is imported, and nothing that isn't marked for import should be imported.


The IRLinker necessarily needs to link in the definition of any local referenced from the module being linked. This is necessary for both Full LTO, as well as for ThinLTO if we decided not to promote and want to import a copy. For ThinLTO we control this by promoting before the IRLinker is invoked (by invoking renameModuleForThinLTO) - so if it has been promoted then the IRLinker sees a non-local and does not force the def to be linked, but if it is still local at that point we need to import a local copy.

Note that this is still going to be the same when we change the local const heuristic to be decided in the thin link instead of during the backend's renaming logic, based on a new flag to be added indicating constness. In that case the local would not be marked as a global in the index, and we will not promote in renameModuleForThinLTO (superseding the old heuristic in doPromoteLocalToGlobal), and the IRLinker will import a local copy since it will still be local.


https://reviews.llvm.org/D26880