[PATCH] D35261: Change GlobalValueSummaryMapTy from std::map to DenseMap.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Analysis/ModuleSummaryAnalysis.h:32
 /// that information.
-ModuleSummaryIndex buildModuleSummaryIndex(
+ModuleSummaryIndex *buildModuleSummaryIndex(
     const Module &M,
----------------
I take it this change was necessary because buildModuleSummaryIndex indirectly emits references to the elements of the map? (& DenseMap doesn't preserve reference/pointer validity over move? That surprises me slightly, but I could believe it)

Is that correct? (otherwise - what's the motivation for this change?)

This should be unique_ptr<ModuleSummaryIndex> not raw-but-owning pointer ModuleSummaryIndex*.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:88-114
 struct ValueInfo {
-  const GlobalValueSummaryMapTy::value_type *Ref = nullptr;
+  GlobalValue::GUID GUID = 0;
+  const GlobalValueSummaryMapTy *Map = nullptr;
 
   ValueInfo() = default;
-  ValueInfo(const GlobalValueSummaryMapTy::value_type *Ref) : Ref(Ref) {}
+  ValueInfo(GlobalValue::GUID GUID, const GlobalValueSummaryMapTy *Map)
+      : GUID(GUID), Map(Map) {}
----------------
I take it this change is necessary because ValueInfo elements are stored over mutations to the ModuleSummaryIndex & thus would invalidate references/pointers/iterators to elements & so the map lookup must be deferred?

Is that correct? Is there a particular/canonical/unavoidable example of this usage?


================
Comment at: lib/LTO/LTO.cpp:1058
 
-    std::set<GlobalValue::GUID> ExportedGUIDs;
+    DenseSet<GlobalValue::GUID> ExportedGUIDs;
     for (auto &Res : GlobalResolutions) {
----------------
This looks like an independent change? Should it be submitted separately with the value/memory usage/runtime performance improvements appropriately credited (so it's clear how much of the benefit is from this change as opposed to the ModuleSummaryIndex change?)


https://reviews.llvm.org/D35261