Address review comments and formatting issues

orodley · orodley · commit d3bac46f2554 · 2026-03-13T03:17:55.000Z
diff --git a/llvm/include/llvm/IR/GlobalValue.h b/llvm/include/llvm/IR/GlobalValue.h
@@ -615,6 +615,10 @@ class GlobalValue : public Constant {
   /// Return a 64-bit global unique ID for this value. It is based on the
   /// "original" name and linkage of this value (i.e. whenever its GUID was
   /// assigned). This might not match the current name and linkage.
+  ///
+  /// The \c AssignGUIDPass must be run before this is called, otherwise
+  /// GUIDs won't be available. This pass can be run multiple times as it does
+  /// nothing if GUID metadata is already present.
   GUID getGUID() const;
 
   /// Return the GUID for this value if it has been assigned, nullopt
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
@@ -577,21 +577,28 @@ class LLVM_ABI Module {
   // Use global_size() to get the total number of global variables.
   // Use globals() to get the range of all global variables.
 
-  std::optional<GlobalValue::GUID> getGUID(const Value* V) const {
+  std::optional<GlobalValue::GUID> getGUID(const Value *V) const {
     const auto It = ValueToGUIDMap.find(V);
     if (It == ValueToGUIDMap.end()) {
-      return {};
+      return std::nullopt;
     }
 
     return It->getSecond();
   }
 
-  void insertGUID(const Value* V, GlobalValue::GUID GUID) {
-    ValueToGUIDMap[V] = GUID;
+  void insertGUID(const Value *V, GlobalValue::GUID GUID) {
+    const auto [It, WasInserted] = ValueToGUIDMap.insert({V, GUID});
+    
+    (void)It, (void)WasInserted;
+  #ifndef NDEBUG
+    if (!WasInserted) {
+      assert((It->second == GUID) && "insertGUID called with different value");
+    }
+  #endif
   }
 
 private:
-  DenseMap<const Value*, GlobalValue::GUID> ValueToGUIDMap;
+  DenseMap<const Value *, GlobalValue::GUID> ValueToGUIDMap;
 
 /// @}
 /// @name Direct access to the globals list, functions list, and symbol table
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -1770,8 +1770,10 @@ class ModuleSummaryIndex {
     return ValueInfo(HaveGVs, VP);
   }
 
-  /// Return a ValueInfo for \p GV with GUID \p GUID and mark it as belonging to GV.
-  ValueInfo getOrInsertValueInfo(const GlobalValue *GV, GlobalValue::GUID GUID) {
+  /// Return a ValueInfo for \p GV with GUID \p GUID and mark it as belonging to
+  /// GV.
+  ValueInfo getOrInsertValueInfo(const GlobalValue *GV,
+                                 GlobalValue::GUID GUID) {
     assert(HaveGVs);
     auto VP = getOrInsertValuePtr(GUID);
     VP->second.U.GV = GV;
diff --git a/llvm/lib/Analysis/CtxProfAnalysis.cpp b/llvm/lib/Analysis/CtxProfAnalysis.cpp
@@ -50,7 +50,7 @@ static cl::opt<bool> ForceIsInSpecializedModule(
     "ctx-profile-force-is-specialized", cl::init(false),
     cl::desc("Treat the given module as-if it were containing the "
              "post-thinlink module containing the root"));
- 
+
 class ProfileAnnotatorImpl final {
   friend class ProfileAnnotator;
   class BBInfo;
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
@@ -9851,7 +9851,9 @@ bool LLParser::addGlobalValueToIndex(
       // Be a little lenient here, to accomodate older files without GUIDs
       // already computed and assigned as metadata.
       auto MaybeGUID = GV->getGUIDIfAssigned();
-      GUID = MaybeGUID ? *MaybeGUID : GlobalValue::getGUIDAssumingExternalLinkage(GV->getName());
+      GUID = MaybeGUID
+                 ? *MaybeGUID
+                 : GlobalValue::getGUIDAssumingExternalLinkage(GV->getName());
 
       VI = Index->getOrInsertValueInfo(GV, GUID);
     } else {
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -978,6 +978,8 @@ class ModuleSummaryIndexBitcodeReader : public BitcodeReaderBase {
   /// Callback to ask whether a symbol is the prevailing copy when invoked
   /// during combined index building.
   std::function<bool(StringRef)> IsPrevailing = nullptr;
+  
+  /// Callback invoked whenever a new ValueInfo is generated.
   std::function<void(ValueInfo)> OnValueInfo = nullptr;
 
   /// Saves the stack ids from the STACK_IDS record to consult when adding
@@ -993,9 +995,9 @@ class ModuleSummaryIndexBitcodeReader : public BitcodeReaderBase {
   /// list and used to avoid repeated hash lookups.
   std::vector<unsigned> StackIdToIndex;
 
-   /// A list of GUIDs defined by this module. Indexed by ValueID.
-   std::vector<uint64_t> DefinedGUIDs;
- 
+  /// A list of GUIDs defined by this module. Indexed by ValueID.
+  std::vector<uint64_t> DefinedGUIDs;
+
 public:
   ModuleSummaryIndexBitcodeReader(
       BitstreamCursor Stream, StringRef Strtab, ModuleSummaryIndex &TheIndex,
@@ -4050,9 +4052,10 @@ Error BitcodeReader::globalCleanup() {
 
   for (size_t ValueID = 0; ValueID < GUIDList.size(); ValueID++) {
     const auto GUID = GUIDList[ValueID];
-    if (GUID == 0) continue;
+    if (GUID == 0)
+      continue;
 
-    const auto* Value = ValueList[ValueID];
+    const auto *Value = ValueList[ValueID];
     TheModule->insertGUID(Value, GUID);
   }
 
@@ -7282,13 +7285,14 @@ void ModuleSummaryIndexBitcodeReader::setValueGUID(
     ValueGUID = DefinedGUIDs[ValueID];
   if (ValueGUID == 0)
     ValueGUID = GlobalValue::getGUIDAssumingExternalLinkage(
-      GlobalValue::getGlobalIdentifier(ValueName, Linkage, SourceFileName));
+        GlobalValue::getGlobalIdentifier(ValueName, Linkage, SourceFileName));
 
   auto OriginalNameID = ValueGUID;
   if (GlobalValue::isLocalLinkage(Linkage))
     OriginalNameID = GlobalValue::getGUIDAssumingExternalLinkage(ValueName);
   if (PrintSummaryGUIDs)
-    dbgs() << "GUID " << ValueGUID << "(" << OriginalNameID << ") is " << ValueName << "\n";
+    dbgs() << "GUID " << ValueGUID << "(" << OriginalNameID << ") is "
+           << ValueName << "\n";
 
   // UseStrtab is false for legacy summary formats and value names are
   // created on stack. In that case we save the name in a string saver in
@@ -8720,10 +8724,10 @@ BitcodeModule::getLazyModule(LLVMContext &Context, bool ShouldLazyLoadMetadata,
 // We don't use ModuleIdentifier here because the client may need to control the
 // module path used in the combined summary (e.g. when reading summaries for
 // regular LTO modules).
-Error BitcodeModule::readSummary(
-    ModuleSummaryIndex &CombinedIndex, StringRef ModulePath,
-    std::function<bool(StringRef)> IsPrevailing,
-    std::function<void(ValueInfo)> OnValueInfo) {
+Error BitcodeModule::readSummary(ModuleSummaryIndex &CombinedIndex,
+                                 StringRef ModulePath,
+                                 std::function<bool(StringRef)> IsPrevailing,
+                                 std::function<void(ValueInfo)> OnValueInfo) {
   BitstreamCursor Stream(Buffer);
   if (Error JumpFailed = Stream.JumpToBit(ModuleBit))
     return JumpFailed;
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -4592,7 +4592,8 @@ void ModuleBitcodeWriterBase::writePerModuleFunctionSummaryRecord(
 
 static GlobalValue::GUID getOrComputeGUID(const GlobalValue& V) {
   auto MaybeGUID = V.getGUIDIfAssigned();
-  return MaybeGUID ? *MaybeGUID : GlobalValue::getGUIDAssumingExternalLinkage(V.getName());
+  return MaybeGUID ? *MaybeGUID
+                   : GlobalValue::getGUIDAssumingExternalLinkage(V.getName());
 }
 
 // Collect the global value references in the given variable's initializer,
@@ -4919,7 +4920,8 @@ void ModuleBitcodeWriterBase::writeGUIDList() {
     } else {
       GUID = 0;
     }
-    if (GUID == 0) continue;
+    if (GUID == 0)
+      continue;
 
     const auto ValueID = VE.getValueID(&GV);
     GUIDs[ValueID] = GUID;
diff --git a/llvm/lib/IR/Globals.cpp b/llvm/lib/IR/Globals.cpp
@@ -103,19 +103,23 @@ std::optional<GlobalValue::GUID> GlobalValue::getGUIDIfAssigned() const {
   auto *MD = getMetadata(LLVMContext::MD_unique_id);
   if (MD != nullptr)
     return cast<ConstantInt>(cast<ConstantAsMetadata>(MD->getOperand(0))
-                                ->getValue()
-                                ->stripPointerCasts())
+                                 ->getValue()
+                                 ->stripPointerCasts())
         ->getZExtValue();
 
   // Handle a few special cases where we just want to compute it based on the
   // current properties.
-  if (isDeclaration() || isa<GlobalAlias>(this) || getName().starts_with("llvm.")) {
+  // TODO: Maybe we should use a more robust check for intrinsics than just
+  // matching on the name?
+  if (isDeclaration() || isa<GlobalAlias>(this) ||
+      getName().starts_with("llvm.")) {
     return GlobalValue::getGUIDAssumingExternalLinkage(getGlobalIdentifier());
   }
-  
+
   // Otherwise we try to look it up in the module, for cases where we've read
   // the GUID table but not the metadata.
-  if (getParent() == nullptr) return {};
+  if (getParent() == nullptr)
+    return {};
   const Module &M = *getParent();
 
   return M.getGUID(this);
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
@@ -1209,8 +1209,7 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
         // Find the summary in the module for this very GV and record the new
         // linkage so that we can switch it when we import the GV.
         if (R.LinkerRedefined)
-          if (auto *S = ThinLTO.CombinedIndex.findSummaryInModule(
-                  GUID, BMID))
+          if (auto *S = ThinLTO.CombinedIndex.findSummaryInModule(GUID, BMID))
             S->setLinkage(GlobalValue::WeakAnyLinkage);
       }
 
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
@@ -609,7 +609,8 @@ static void dropDeadSymbols(Module &Mod, const GVSummaryMapTy &DefinedGlobals,
 
   for (auto &GV : Mod.global_values()) {
     auto GUID = Mod.getGUID(&GV);
-    if (!GUID) continue;
+    if (!GUID)
+      continue;
 
     if (GlobalValueSummary *GVS = DefinedGlobals.lookup(*GUID))
       if (!Index.isGlobalValueLive(GVS)) {
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -1879,19 +1879,18 @@ static void internalizeGVsAfterImport(Module &M) {
     }
 }
 
-static GlobalValue::GUID getGUIDOrFallback(const GlobalValue& GV) {
-    // Modules compiled against the current version should have a GUID table
-    // and hence have the GUID available simply by calling getGUID(). But we
-    // want to support running on old bitcode files which have no embedded
-    // GUIDs. So if it's missing we fall back to computing the GUID with its
-    // current name / linkage type.
-    const auto MaybeGUID = GV.getGUIDIfAssigned();
-    return MaybeGUID ? *MaybeGUID
-        : GlobalValue::getGUIDAssumingExternalLinkage(
-            GlobalValue::getGlobalIdentifier(
-              GV.getName(),
-              GV.getLinkage(),
-              GV.getParent()->getSourceFileName()));
+static GlobalValue::GUID getGUIDOrFallback(const GlobalValue &GV) {
+  // Modules compiled against the current version should have a GUID table
+  // and hence have the GUID available simply by calling getGUID(). But we
+  // want to support running on old bitcode files which have no embedded
+  // GUIDs. So if it's missing we fall back to computing the GUID with its
+  // current name / linkage type.
+  const auto MaybeGUID = GV.getGUIDIfAssigned();
+  return MaybeGUID ? *MaybeGUID
+                   : GlobalValue::getGUIDAssumingExternalLinkage(
+                         GlobalValue::getGlobalIdentifier(
+                             GV.getName(), GV.getLinkage(),
+                             GV.getParent()->getSourceFileName()));
 }
 
 // Automatically import functions in Module \p DestModule based on the summaries
diff --git a/llvm/lib/Transforms/Utils/AssignGUID.cpp b/llvm/lib/Transforms/Utils/AssignGUID.cpp
@@ -6,9 +6,10 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file provides a pass which assigns a a GUID (globally unique identifier)
-// to every GlobalValue in the module, according to its current name, linkage,
-// and originating file.
+// This file provides a pass which assigns GUID (globally unique identifier)
+// metadata to every GlobalValue in the module, according to its current name,
+// linkage, and originating file. It is idempotent -- if GUID metadata is
+// already present, it does nothing.
 //
 //===----------------------------------------------------------------------===//
 
diff --git a/llvm/lib/Transforms/Utils/CloneModule.cpp b/llvm/lib/Transforms/Utils/CloneModule.cpp
@@ -143,7 +143,7 @@ std::unique_ptr<Module> llvm::CloneModule(
     }
 
     if (!ShouldCloneDefinition(&I)) {
-      CopyMD();            
+      CopyMD();
       // Skip after setting the correct linkage for an external reference.
       F->setLinkage(GlobalValue::ExternalLinkage);
       // Personality function is not valid on a declaration.
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -166,19 +166,18 @@ FunctionImportGlobalProcessing::getPromotedName(const GlobalValue *SGV) {
       ImportIndex.getModuleHash(SGV->getParent()->getModuleIdentifier()));
 }
 
-static GlobalValue::GUID getGUIDOrFallback(const GlobalValue& GV) {
-    // Modules compiled against the current version should have a GUID table
-    // and hence have the GUID available simply by calling getGUID(). But we
-    // want to support running on old bitcode files which have no embedded
-    // GUIDs. So if it's missing we fall back to computing the GUID with its
-    // current name / linkage type.
-    const auto MaybeGUID = GV.getGUIDIfAssigned();
-    return MaybeGUID ? *MaybeGUID
-        : GlobalValue::getGUIDAssumingExternalLinkage(
-            GlobalValue::getGlobalIdentifier(
-              GV.getName(),
-              GV.getLinkage(),
-              GV.getParent()->getSourceFileName()));
+static GlobalValue::GUID getGUIDOrFallback(const GlobalValue &GV) {
+  // Modules compiled against the current version should have a GUID table
+  // and hence have the GUID available simply by calling getGUID(). But we
+  // want to support running on old bitcode files which have no embedded
+  // GUIDs. So if it's missing we fall back to computing the GUID with its
+  // current name / linkage type.
+  const auto MaybeGUID = GV.getGUIDIfAssigned();
+  return MaybeGUID ? *MaybeGUID
+                   : GlobalValue::getGUIDAssumingExternalLinkage(
+                         GlobalValue::getGlobalIdentifier(
+                             GV.getName(), GV.getLinkage(),
+                             GV.getParent()->getSourceFileName()));
 }
 
 GlobalValue::LinkageTypes
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
@@ -361,7 +361,9 @@ static bool importFunctions(const char *argv0, Module &DestModule) {
     // FIXME: A follow-up patch should add test coverage for import declaration
     // in `llvm-link` CLI (e.g., by introducing a new command line option).
     const auto MaybeGUID = F->getGUIDIfAssigned();
-    const auto GUID = MaybeGUID ? *MaybeGUID : GlobalValue::getGUIDAssumingExternalLinkage(F->getName());
+    const auto GUID =
+        MaybeGUID ? *MaybeGUID
+                  : GlobalValue::getGUIDAssumingExternalLinkage(F->getName());
     ImportList.addDefinition(
         FileNameStringCache.insert(FileName).first->getKey(), GUID);
   }

Original file line number	Diff line number	Diff line change
`@@ -1209,8 +1209,7 @@ LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,`
`1209`	`1209`	`// Find the summary in the module for this very GV and record the new`
`1210`	`1210`	`// linkage so that we can switch it when we import the GV.`
`1211`	`1211`	`if (R.LinkerRedefined)`
`1212`		`- if (auto *S = ThinLTO.CombinedIndex.findSummaryInModule(`
`1213`		`- GUID, BMID))`
	`1212`	`+ if (auto *S = ThinLTO.CombinedIndex.findSummaryInModule(GUID, BMID))`
`1214`	`1213`	`S->setLinkage(GlobalValue::WeakAnyLinkage);`
`1215`	`1214`	`}`
`1216`	`1215`