[PATCH] D74751: [ThinLTO] Drop dso_local if a GlobalVariable satisfies isDeclarationForLinker()

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:214
 
-  FunctionImporter Importer(Index, Loader);
+  FunctionImporter Importer(Index, Loader, false);
   Expected<bool> Result = Importer.importFunctions(TheModule, ImportList);
----------------
MaskRay wrote:
> tejohnson wrote:
> > MaskRay wrote:
> > > tejohnson wrote:
> > > > It seems like we'd want to do the same thing here in the old LTO API whenever we have a TM (some of the callers of these routines do).
> > > Added some comments.
> > > 
> > > May I ask what is the old LTO API? Are most functions in ThinLTOCodeGenerator.cpp related to the old LTO API?
> > > 
> > > I try passing ClearDSOLocalOnDeclarations as long as the information is available. `llvm-lto.cpp:707` `ThinGenerator.crossModuleImport(*TheModule, *Index, *Input);` does not pass the information.
> > The old LTO API is used by linkers such as ld64 and Sony's proprietary linker and possibly others. I'm not sure if there are any that are ELF, but good to keep this up to date there as well since we still support both.
> > 
> > A few of these methods are only used for testing by llvm-lto, where we don't have a TM, so it's fine to skip it there.
> It seems that the D35702 logic does not apply to the old API. `GlobalValueSummary::setDSOLocal` is never called.
> 
> ```lang=cpp
> // lib/LTO/LTO.cpp
>       if (Res.FinalDefinitionInLinkageUnit) {
>         if (auto S = ThinLTO.CombinedIndex.findSummaryInModule(
>                 GUID, BM.getModuleIdentifier())) {
>           S->setDSOLocal(true);
>         }
>       }
> ```
> 
> Nevertheless, I added 
> 
> ```
> ; RUN: llvm-lto -thinlto-action=run %t1.bc %t2.bc -thinlto-save-temps=%t5.
> ; RUN: llvm-dis < %t5.1.3.imported.bc | FileCheck %s --check-prefix=OLDAPI_SRC
> ```
> 
I see. Good to have the fix / test in place in case it ever gets supported there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74751