[PATCH] D95943: [ThinLTO] Fix import of a global variable to modules contain its weak def

Wed Feb 10 09:41:14 PST 2021

tejohnson added a comment.

In D95943#2548382 <https://reviews.llvm.org/D95943#2548382>, @krisb wrote:

> Thanks, @tejohnson, for reviewing this!
> I'll make the comment more accurate.
>
>> I think we would still want to check linkage type, since it is only interposable that are converted to declarations. The prevailing information would be useful to refine the extra importing further. Hmm, actually I think there might be a correctness issue with this fix as is. I notice in your test below that the imported symbol becomes internalized. I believe this will need to stay interposable. Normally interposable symbols are blocked from inlining and other IPO since they can be overridden at link time. By making the imported copy internalized, it can now be inlined and no longer overridden at link time. If we did ensure that we imported the prevailing copy this might be ok. However, there are going to be cases where that is not possible (e.g. doing ThinLTO on a shared library link, or if the prevailing copy is in an ELF object/shared library and therefore not available for importing). So I think the best thing here is to ensure that when it gets imported it retains the weak linkage type. I'm not sure if is being internalized in this particular test case because the imported copy is read only - can you confirm where/how that is happening?
>
> The imported copy becomes internal because the source def is read-only indeed (the imported copy inherits 'thinlto-internalize' attribute from the source one thus it got internalized in internalizeGVsAfterImport()). This seems okay since access from an ELF object/shared library prevents the source copy to be marked ReadOnly and at that time we already have attributes propagated.
> In general, allow importing here we let it follow the normal importing rules as if the destination module doesn't contain a def at all. So that we will not import another copy with interposable linkage (it doesn't pass the check from canImportGlobalVar()). For other cases, the final linkage type will be whatever FunctionImportGlobalProcessing::getLinkage() returns for the source copy. Unless I'm missing something, this seems to be safe.

Ok I think I have convinced myself that this is correct:

1. If the prevailing copy is not available for import (e.g. in an ELF object), these IR copies would have been marked dead and therefore we would not have marked them as read only.
2. Any copy available for import, i.e. not interposable, would have to be a linkage type that cannot be overridden. So it would have to either be the prevailing copy, or one odr equivalent.

Hmm, it seems like we could do something similar for function importing, to allow importing and inlining of the prevailing copy when the copy in the module is interposable. Can you add a FIXME where we check "if (DefinedGVSummaries.count(VI.getGUID()))" in computeImportForFunction?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95943/new/

https://reviews.llvm.org/D95943