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

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D26880#601056, @tejohnson wrote:

> 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:
> > >
> > > >
> > >
> > >
> > > 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.


Not necessarily: it can just abort and return an error.

> This is necessary for both Full LTO, as well as for ThinLTO if we decided not to promote and want to import a copy.

If we decide to import a copy, we should ask the IRLinker to import this local.

> 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.

What you're describing is that we *can't* express the copy at this point, so we leave the IRLinker implicitly handling it.
I'm against any form of implicit: it adds complexity and makes it too hard to reason about each components and the invariant of the system by "hiding" them from the APIs.
This is what lead to bugs like the one addressed in this current patch.

In https://reviews.llvm.org/D26880#601057, @tejohnson wrote:

> In https://reviews.llvm.org/D26880#600928, @mehdi_amini wrote:
>
> > I have a different approach: 1) this should go in in any case, because correctness first,
>
>
> This patch will not be correct on it's own, it can't go in without removing the logic in doPromoteLocalToGlobal.


Then we're missing a test case in the validation, as it is passing with this patch.

> 
> 
>> and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.
> 
> I'm concerned about the performance we lose in the meantime.

I'm not why it should be a priority? Development on trunk should focus on correctness of the system *before* optimizations concerns.


https://reviews.llvm.org/D26880