[ThinLTO] Avoid extra index lookup during promotion

[tejohnson]

Summary

Pass down the already accessed ValueInfo to shouldPromoteLocalToGlobal,
to avoid an unnecessary extra index lookup.

Add some assertion checking to confirm we have a non-empty VI when
expected.

Also some misc cleanup, merging the two versions of
doImportAsDefinition, since one was only called by the other, and
unnecessarily passed in a member variable.

[evgeny777]

LGTM. BTW, do you have any data on performance improvement?

[tejohnson]

In D70337#1749277, @evgeny777 wrote:

LGTM. BTW, do you have any data on performance improvement?

No. It was just something I noticed when working on a patch that needed to get the VI for another reason in this code.

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

class FunctionImportGlobalProcessing {
   SmallPtrSet<GlobalValue *, 8> Used;
 
   /// Keep track of any COMDATs that require renaming (because COMDAT
   /// leader was promoted and renamed). Maps from original COMDAT to one
   /// with new name.
   DenseMap<const Comdat *, Comdat *> RenamedComdats;
 
   /// Check if we should promote the given local value to global scope.
-  bool shouldPromoteLocalToGlobal(const GlobalValue *SGV);
+  bool shouldPromoteLocalToGlobal(const GlobalValue *SGV, ValueInfo VI);
 
 #ifndef NDEBUG
   /// Check if the given value is a local that can't be renamed (promoted).
   /// Only used in assertion checking, and disabled under NDEBUG since the Used
   /// set will not be populated.
   bool isNonRenamableLocal(const GlobalValue &GV) const;
 #endif
 
#ifndef NDEBUG
     // First collect those in the llvm.used set.
     collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);
     // Next collect those in the llvm.compiler.used set.
     collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ true);
 #endif
   }
 
   bool run();
-
-  static bool doImportAsDefinition(const GlobalValue *SGV,
-                                   SetVector<GlobalValue *> *GlobalsToImport);
 };
 
 /// Perform in-place global value handling on the given Module for
 /// exported local functions renamed and promoted for ThinLTO.
 bool renameModuleForThinLTO(
     Module &M, const ModuleSummaryIndex &Index,
     SetVector<GlobalValue *> *GlobalsToImport = nullptr);
 
 /// Compute synthetic function entry counts.
 void computeSyntheticCounts(ModuleSummaryIndex &Index);
 
 } // End llvm namespace
 
 #endif

llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/InstIterator.h"
 using namespace llvm;
 
 /// Checks if we should import SGV as a definition, otherwise import as a
 /// declaration.
 bool FunctionImportGlobalProcessing::doImportAsDefinition(
-    const GlobalValue *SGV, SetVector<GlobalValue *> *GlobalsToImport) {
+    const GlobalValue *SGV) {
+  if (!isPerformingImport())
+    return false;
 
   // Only import the globals requested for importing.
   if (!GlobalsToImport->count(const_cast<GlobalValue *>(SGV)))
     return false;
 
   assert(!isa<GlobalAlias>(SGV) &&
          "Unexpected global alias in the import list.");
 
   // Otherwise yes.
   return true;
 }
 
-bool FunctionImportGlobalProcessing::doImportAsDefinition(
-    const GlobalValue *SGV) {
-  if (!isPerformingImport())
-    return false;
-  return FunctionImportGlobalProcessing::doImportAsDefinition(SGV,
-                                                              GlobalsToImport);
-}
-
 bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(
-    const GlobalValue *SGV) {
+    const GlobalValue *SGV, ValueInfo VI) {
   assert(SGV->hasLocalLinkage());
   // Both the imported references and the original local variable must
   // be promoted.
   if (!isPerformingImport() && !isModuleExporting())
     return false;
 
   if (isPerformingImport()) {
     assert((!GlobalsToImport->count(const_cast<GlobalValue *>(SGV)) ||
             !isNonRenamableLocal(*SGV)) &&
            "Attempting to promote non-renamable local");
     // We don't know for sure yet if we are importing this value (as either
     // a reference or a def), since we are simply walking all values in the
     // module. But by necessity if we end up importing it and it is local,
     // it must be promoted, so unconditionally promote all values in the
     // importing module.
     return true;
   }
 
   // When exporting, consult the index. We can have more than one local
   // with the same GUID, in the case of same-named locals in different but
   // same-named source files that were compiled in their respective directories
   // (so the source file name and resulting GUID is the same). Find the one
   // in this module.
   auto Summary = ImportIndex.findSummaryInModule(
-      SGV->getGUID(), SGV->getParent()->getModuleIdentifier());
+      VI, SGV->getParent()->getModuleIdentifier());
   assert(Summary && "Missing summary for global value when exporting");
   auto Linkage = Summary->linkage();
   if (!GlobalValue::isLocalLinkage(Linkage)) {
     assert(!isNonRenamableLocal(*SGV) &&
            "Attempting to promote non-renamable local");
     return true;
   }
 
if (GV.hasName()) {
     // definition.
     if (VI && VI.isDSOLocal()) {
       GV.setDSOLocal(true);
       if (GV.hasDLLImportStorageClass())
         GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
     }
   }
 
+  // We should always have a ValueInfo (i.e. GV in index) for definitions when
+  // we are exporting, and also when importing that value.
+  assert(VI || GV.isDeclaration() ||
+         (isPerformingImport() && !doImportAsDefinition(&GV)));
+
   // Mark read/write-only variables which can be imported with specific
   // attribute. We can't internalize them now because IRMover will fail
   // to link variable definitions to their external declarations during
   // ThinLTO import. We'll internalize read-only variables later, after
   // import is finished. See internalizeGVsAfterImport.
   //
   // If global value dead stripping is not enabled in summary then
   // propagateConstants hasn't been run. We can't internalize GV
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()));
       }
     }
   }
 
-  if (GV.hasLocalLinkage() && shouldPromoteLocalToGlobal(&GV)) {
+  if (GV.hasLocalLinkage() && shouldPromoteLocalToGlobal(&GV, VI)) {
     // Save the original name string before we rename GV below.
     auto Name = GV.getName().str();
     GV.setName(getPromotedName(&GV));
     GV.setLinkage(getLinkage(&GV, /* DoPromote */ true));
     assert(!GV.hasLocalLinkage());
     GV.setVisibility(GlobalValue::HiddenVisibility);
 
     // If we are renaming a COMDAT leader, ensure that we record the COMDAT