[ThinLTO] Use NoPromote flag in summary during promotion

[tejohnson]

Summary

Replace the check of whether a GV has a section with the flag check
in the summary. This is in preparation for using the NoPromote flag
to convey other situations when we can't promote (e.g. locals used in
inline asm).

[mehdi_amini]

include/llvm/Transforms/Utils/FunctionImportUtils.h

58 ↗

(On Diff #76163)

Update doc.

70 ↗

(On Diff #76163)

Ditto.

lib/Transforms/Utils/FunctionImportUtils.cpp

206 ↗

(On Diff #76163)

The flow to initialize the "DoPromote" is very difficult to process / follow here. We should seek some refactoring to make it more readable.

[tejohnson]

lib/Transforms/Utils/FunctionImportUtils.cpp

206 ↗

(On Diff #76163)

I passed in false here directly, since DoPromote would always be false and that will hopefully be clearer.

[tejohnson]

Address review comments

[mehdi_amini]

LGTM.

include/llvm/Transforms/Utils/FunctionImportUtils.h

44 ↗

(On Diff #76180)

The name should be "shouldPromoteLocalToGlobal", the "do" added some cognitive load for me to read this patch (I'm not saying that it should be done as part of this patch, but either before or after would be a nice tweak).

lib/Transforms/Utils/FunctionImportUtils.cpp

70 ↗

(On Diff #76180)

This SGV->getGUID() is the cause for the "DoPromote" right?

[tejohnson]

include/llvm/Transforms/Utils/FunctionImportUtils.h

44 ↗

(On Diff #76180)

Sure. I have this one ready to go, so will do the rename in an immediately following patch.

lib/Transforms/Utils/FunctionImportUtils.cpp

70 ↗

(On Diff #76180)

Right.

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

class FunctionImportGlobalProcessing {
   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.
-  std::string getName(const GlobalValue *SGV);
+  /// 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);
 
   /// 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);
 
   /// Get the new linkage for SGV that should be used in the linked destination
   /// module. Specifically, for ThinLTO importing or exporting it may need
-  /// to be adjusted.
-  GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV);
+  /// to be adjusted. When \p DoPromote is true then we must adjust the
+  /// linkage for a required promotion of a local to global scope.
+  GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV, bool DoPromote);
 
 public:
   FunctionImportGlobalProcessing(
       Module &M, const ModuleSummaryIndex &Index,
       DenseSet<const GlobalValue *> *GlobalsToImport = nullptr)
       : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport) {
     // If we have a ModuleSummaryIndex but no function to import,
     // then this is the primary module being compiled in a ThinLTO

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

bool FunctionImportGlobalProcessing::doPromoteLocalToGlobal(
   // address taken by checking the unnamed addr flag. To be more aggressive,
   // the address taken information must be checked earlier during parsing
   // of the module and recorded in the summary index for use when importing
   // from that module.
   auto *GVar = dyn_cast<GlobalVariable>(SGV);
   if (GVar && GVar->isConstant() && GVar->hasGlobalUnnamedAddr())
     return false;
 
-  if (GVar && GVar->hasSection())
-    // Some sections like "__DATA,__cfstring" are "magic" and promotion is not
-    // allowed. Just disable promotion on any GVar with sections right now.
+  auto *Summary = ImportIndex.getGlobalValueSummary(SGV->getGUID());
+  assert(Summary && "Missing summary for global value");
+  if (Summary->noRename())
     return false;
 
   // Eventually we only need to promote functions in the exporting module that
   // are referenced by a potentially exported function (i.e. one that is in the
   // summary index).
   return true;
 }
 
-std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV) {
+std::string FunctionImportGlobalProcessing::getName(const GlobalValue *SGV,
+                                                    bool DoPromote) {
   // 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() &&
-      (doPromoteLocalToGlobal(SGV) || isPerformingImport()))
+  if (SGV->hasLocalLinkage() && (DoPromote || isPerformingImport()))
     return ModuleSummaryIndex::getGlobalNameForLocal(
         SGV->getName(),
         ImportIndex.getModuleHash(SGV->getParent()->getModuleIdentifier()));
   return SGV->getName();
 }
 
 GlobalValue::LinkageTypes
-FunctionImportGlobalProcessing::getLinkage(const GlobalValue *SGV) {
+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
   // all as potentially exported if this module is exporting anything.
   if (isModuleExporting()) {
-    if (SGV->hasLocalLinkage() && doPromoteLocalToGlobal(SGV))
+    if (SGV->hasLocalLinkage() && DoPromote)
       return GlobalValue::ExternalLinkage;
     return SGV->getLinkage();
   }
 
   // Otherwise, if we aren't importing, no linkage change is needed.
   if (!isPerformingImport())
     return SGV->getLinkage();
 
case GlobalValue::AppendingLinkage:
     // by linkIfNeeded, and we will assert in shouldLinkFromSource
     // if we try to import, so we simply return AppendingLinkage.
     return GlobalValue::AppendingLinkage;
 
   case GlobalValue::InternalLinkage:
   case GlobalValue::PrivateLinkage:
     // If we are promoting the local to global scope, it is handled
     // similarly to a normal externally visible global.
-    if (doPromoteLocalToGlobal(SGV)) {
+    if (DoPromote) {
       if (doImportAsDefinition(SGV) && !dyn_cast<GlobalAlias>(SGV))
         return GlobalValue::AvailableExternallyLinkage;
       else
         return GlobalValue::ExternalLinkage;
     }
     // A non-promoted imported local definition stays local.
     // The ThinLTO pass will eventually force-import their definitions.
     return SGV->getLinkage();
case GlobalValue::CommonLinkage:
     // The ThinLTO pass will eventually force-import their definitions.
     return SGV->getLinkage();
   }
 
   llvm_unreachable("unknown linkage type");
 }
 
 void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
+  bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
-      (doPromoteLocalToGlobal(&GV) || isPerformingImport())) {
-    GV.setName(getName(&GV));
-    GV.setLinkage(getLinkage(&GV));
+      ((DoPromote = doPromoteLocalToGlobal(&GV)) || isPerformingImport())) {
+    // Once we change the name or linkage it is difficult to determine
+    // again whether we should promote since doPromoteLocalToGlobal 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);
   } else
-    GV.setLinkage(getLinkage(&GV));
+    GV.setLinkage(getLinkage(&GV, /* DoPromote */ false));
 
   // Remove functions imported as available externally defs from comdats,
   // as this is a declaration for the linker, and will be dropped eventually.
   // It is illegal for comdats to contain declarations.
   auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
   if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
     // The IRMover should not have placed any imported declarations in
     // a comdat, so the only declaration that should be in a comdat