[PATCH] D32471: IR: Use pointers instead of GUIDs to represent edges in the module summary. NFCI.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks pretty good, just a few more comments/questions.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:557
                              std::unique_ptr<GlobalValueSummary> Summary) {
-    addOriginalName(GlobalValue::getGUID(ValueName),
-                    Summary->getOriginalName());
-    GlobalValueMap[GlobalValue::getGUID(ValueName)].push_back(
-        std::move(Summary));
+    addGlobalValueSummary(getOrInsertValueInfo(GlobalValue::getGUID(ValueName)),
+                          std::move(Summary));
----------------
pcc wrote:
> tejohnson wrote:
> > Confused by something here - if getOrInsertValueInfo ends up inserting a new ValueInfo, it's Ref pointer will be null, but the below addGlobalValueSummary will unconditionally dereference it.
> > getOrInsertValueInfo ends up inserting a new ValueInfo, it's Ref pointer will be null,
> 
> That should never be the case. If it inserts, the Ref pointer should refer to the newly created map entry.
You're right, nevermind.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:467
   for (auto &GlobalList : Index) {
-    assert(GlobalList.second.size() == 1 &&
+    if (GlobalList.second.SummaryList.empty())
+      continue;
----------------
pcc wrote:
> tejohnson wrote:
> > Why this added check? Previously we always asserted that we had a list of size 1. Are we inserting GUIDs into the index too eagarly now?
> Yes, when we see a reference we need to add a GUID to the index. If the reference is undefined in the current module the summary list will be empty.
Ah ok, previously the index only held entries for defined globals, now it contains entries for all referenced globals as well. Maybe add a comment above the empty check (checking if the global is defined in this module).


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:705
+  std::vector<std::pair<ValueInfo, GlobalValue::GUID>>
       ValueIdToCallGraphGUIDMap;
 
----------------
Name and description need update. Is there a good reason to switch from a map to a std::vector? It makes the update code messier because you always have to check for a resize.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3274
+  auto VI = Index->getValueInfo(GlobalValue::getGUID(V.getName()));
+  if (!VI || VI.getSummaryList().empty()) {
     // Only declarations should not have a summary (a declaration might however
----------------
When would a declaration have !VI - would that be a declaration that isn't referenced?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:196
+      VI = Index.getValueInfo(GUID);
+      if (!VI)
+        continue;
----------------
What if VI also has an empty summary list?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:436
+    DEBUG(dbgs() << "Live root: " << VI.getGUID() << "\n");
+    Worklist.push_back(VI);
   }
----------------
Can this loop be merged with the earlier one that builds the initial LiveSymbols set?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:472
       if (auto *AS = dyn_cast<AliasSummary>(Summary.get())) {
         auto AliaseeGUID = AS->getAliasee().getOriginalName();
+        ValueInfo AliaseeVI = Index.getValueInfo(AliaseeGUID);
----------------
Should the OriginalName in the AliasSummary also be saved as a ValueInfo instead of GUID (not necessary in this patch, but as a follow on)?


https://reviews.llvm.org/D32471