[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#601477, @mehdi_amini wrote:

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


It's possible that with the new LTO API the full LTO case would work with that handling removed, but I haven't walked through the usage in detail. not sure about the old libLTO case for full LTO.

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

I looked at the test case that was added when the code you are removing here was added (https://reviews.llvm.org/D19102). I think if referencedbyglobal was a local it would have exposed the issue (we would have been importing it and promoting it, with this patch we would lose the promotion).

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

I normally would agree, except that this is an optimization that has been in for awhile, and maintaining it has a simple fix (the patch I sent last night should do it), until we have the longer term enhancement in.


https://reviews.llvm.org/D26880