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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h:42
 
+  bool ClearDSOLocal;
+
----------------
Needs comment. Also, maybe something more specific like ClearDSOLocalOnDeclarations ? Better yet, something that communicates what the condition is like IsPic.


================
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);
----------------
Is a consistent name for the variable.


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1304
   // are potentially exported to other modules.
-  if (renameModuleForThinLTO(M, *Index, nullptr)) {
+  if (renameModuleForThinLTO(M, *Index, false, nullptr)) {
     errs() << "Error renaming module\n";
----------------
document constant parameter, here and elsewhere (I see that is missing for the existing nullptr param, but better to not add more cases like that).


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:276
 
+  if (ClearDSOLocal && GV.isDeclarationForLinker() && !GV.isImplicitDSOLocal())
+    GV.setDSOLocal(false);
----------------
Needs comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74751