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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Needs tests (both llvm-lto and llvm-lto2 to cover both old and new LTO APIs, respectively. For llvm-lto you can trigger the modified code via the -thinlto-action=run option.



================
Comment at: llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h:116
 bool renameModuleForThinLTO(
-    Module &M, const ModuleSummaryIndex &Index,
+    Module &M, const ModuleSummaryIndex &Index, bool DropDSOLocal,
     SetVector<GlobalValue *> *GlobalsToImport = nullptr);
----------------
tejohnson wrote:
> Is a consistent name for the variable.
That should have been "Use" a consistent name for variable btw. Please use "ClearDSOLocalOnDeclarations" for consistency.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:554
 
-  FunctionImporter Importer(CombinedIndex, ModuleLoader);
+  FunctionImporter Importer(CombinedIndex, ModuleLoader, ClearDSOLocalOnDeclarations);
   if (Error Err = Importer.importFunctions(Mod, ImportList).takeError())
----------------
Clang format


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


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:276
 
+  // When ClearDSOLocalOnDeclarations is true, drop dso_local is GV is converted
+  // to a declaration, to disable direct access. Don't do this if GV is
----------------
s/is GV is/if GV is/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74751