[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.

Nice compile time improvements! I didn't get too far, but have a few questions so far.



================
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));
----------------
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.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:68
+        // that we don't bother optimizing them.
+        if (!GV->hasName()) {
+          RefUnnamedGlobal = true;
----------------
We should always be running the NameAnonGlobal pass before we build the index (when preparing for ThinLTO in the pass manager builder, event at -O0) to avoid needing such checks. Are you seeing somewhere that is not happening properly? There seem to be quite a few changes here where the hasName() guard was added.

Hmm, looks like were somewhat inconsistent about this. In several places in this file we assert that hasName(), but I see Davide added a guard to the AllRefsCanBeExternallyReferenced lambda a couple months ago. Can that occur outside of testing? I don't think so but maybe I am missing a case.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:467
   for (auto &GlobalList : Index) {
-    assert(GlobalList.second.size() == 1 &&
+    if (GlobalList.second.SummaryList.empty())
+      continue;
----------------
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?


https://reviews.llvm.org/D32471