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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:214
 
-  FunctionImporter Importer(Index, Loader);
+  FunctionImporter Importer(Index, Loader, false);
   Expected<bool> Result = Importer.importFunctions(TheModule, ImportList);
----------------
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
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74751