[ThinLTO] Promotion handling cleanup (NFC)

[tejohnson]

Summary

Clean up the code that does GV promotion in the ThinLTO backends.

Specifically, we don't need to check whether we are importing since that
is already checked and handled correctly in shouldPromoteLocalToGlobal.
Simply call shouldPromoteLocalToGlobal, and if it returns true we are
guaranteed that we are promoting, whether or not we are importing (or in
the exporting module). This also makes the handling in getName()
consistent with that in getLinkage(), which checks the DoPromote parameter
regardless of whether we are importing or exporting.

[evgeny777]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

271–274

Can getName() be now called with false as second argument? If not may be simplify it as well?

[tejohnson]

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

271–274

Good idea. I cleaned it up to assert that it is passed a local, and renamed to getPromotedName to clarify the behavior.

[tejohnson]

Implement reviewer suggestion and more cleanup

[evgeny777]

LGTM

llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h

#endif
   /// exporting from the current source module.
   bool isPerformingImport() const { return GlobalsToImport != nullptr; }
   bool isModuleExporting() const { return HasExportedFunctions; }
 
   /// If we are importing from the source module, checks if we should
   /// import SGV as a definition, otherwise import as a declaration.
   bool doImportAsDefinition(const GlobalValue *SGV);
 
-  /// Get the name for SGV that should be used in the linked destination
-  /// module. Specifically, this handles the case where we need to rename
-  /// a local that is being promoted to global scope, which it will always
-  /// do when \p DoPromote is true (or when importing a local).
-  std::string getName(const GlobalValue *SGV, bool DoPromote);
+  /// Get the name for a local SGV that should be promoted and renamed to global
+  /// scope in the linked destination module.
+  std::string getPromotedName(const GlobalValue *SGV);
 
   /// Process globals so that they can be used in ThinLTO. This includes
   /// promoting local variables so that they can be reference externally by
   /// thin lto imported globals and converting strong external globals to
   /// available_externally.
   void processGlobalsForThinLTO();
   void processGlobalForThinLTO(GlobalValue &GV);
 

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

bool FunctionImportGlobalProcessing::isNonRenamableLocal(
   if (GV.hasSection())
     return true;
   if (Used.count(const_cast<GlobalValue *>(&GV)))
     return true;
   return false;
 }
 #endif
 
-std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV,
-                                                    bool DoPromote) {
+std::string
+FunctionImportGlobalProcessing::getPromotedName(const GlobalValue *SGV) {
+  assert(SGV->hasLocalLinkage());
   // For locals that must be promoted to global scope, ensure that
   // the promoted name uniquely identifies the copy in the original module,
-  // using the ID assigned during combined index creation. When importing,
-  // we rename all locals (not just those that are promoted) in order to
-  // avoid naming conflicts between locals imported from different modules.
-  if (SGV->hasLocalLinkage() && (DoPromote || isPerformingImport()))
-    return ModuleSummaryIndex::getGlobalNameForLocal(
-        SGV->getName(),
-        ImportIndex.getModuleHash(SGV->getParent()->getModuleIdentifier()));
-  return SGV->getName();
+  // using the ID assigned during combined index creation.
 }
 
 GlobalValue::LinkageTypes
 FunctionImportGlobalProcessing::getLinkage(const GlobalValue *SGV,
                                            bool DoPromote) {
   // Any local variable that is referenced by an exported function needs
   // to be promoted to global scope. Since we don't currently know which
   // functions reference which local variables/functions, we must treat
if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
         // ignore them when computing import. We do not export references
         // of writeonly object. See computeImportForReferencedGlobals
         if (ImportIndex.isWriteOnly(GVS) && GVS->refs().size())
           V->setInitializer(Constant::getNullValue(V->getValueType()));
       }
     }
   }
 
-  bool DoPromote = false;
-  if (GV.hasLocalLinkage() &&
-      ((DoPromote = shouldPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
+  if (GV.hasLocalLinkage() && shouldPromoteLocalToGlobal(&GV)) {
     // Save the original name string before we rename GV below.
     auto Name = GV.getName().str();
-    // Once we change the name or linkage it is difficult to determine
-    // again whether we should promote since shouldPromoteLocalToGlobal needs
-    // to locate the summary (based on GUID from name and linkage). Therefore,
-    // use DoPromote result saved above.
-    GV.setName(getName(&GV, DoPromote));
-    GV.setLinkage(getLinkage(&GV, DoPromote));
-    if (!GV.hasLocalLinkage())
-      GV.setVisibility(GlobalValue::HiddenVisibility);
+    GV.setName(getPromotedName(&GV));
+    GV.setLinkage(getLinkage(&GV, /* DoPromote */ true));
+    assert(!GV.hasLocalLinkage());
 
     // If we are renaming a COMDAT leader, ensure that we record the COMDAT
     // for later renaming as well. This is required for COFF.
     if (const auto *C = GV.getComdat())
       if (C->getName() == Name)
         RenamedComdats.try_emplace(C, M.getOrInsertComdat(GV.getName()));
   } else
     GV.setLinkage(getLinkage(&GV, /* DoPromote */ false));