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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


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


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:68
+        // that we don't bother optimizing them.
+        if (!GV->hasName()) {
+          RefUnnamedGlobal = true;
----------------
tejohnson wrote:
> 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.
This particular change was necessary to handle the test case that Davide added in r295861 because we needed to be able to compute a GUID for any global reference. I discussed the change with him on IRC, and he agreed that we should revert it, so I did that in r301991.

Regarding the other changes related to anonymous globals, they seem unnecessary at this point, so I've undone them as well.


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


https://reviews.llvm.org/D32471