[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/lib/Bitcode/Reader/BitcodeReader.cpp:705
+  std::vector<std::pair<ValueInfo, GlobalValue::GUID>>
       ValueIdToCallGraphGUIDMap;
 
----------------
tejohnson wrote:
> 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.
I saw this as a potential perf improvement, but admittedly I hadn't profiled it in isolation.

I've reverted it for now and if it comes up again we can reconsider.


================
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
----------------
tejohnson wrote:
> When would a declaration have !VI - would that be a declaration that isn't referenced?
Yes, I think that's the only case where that can happen.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:196
+      VI = Index.getValueInfo(GUID);
+      if (!VI)
+        continue;
----------------
tejohnson wrote:
> What if VI also has an empty summary list?
We will fall through and enter selectCallee which will return nullptr, so we will not import anything for this call.


================
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);
----------------
tejohnson wrote:
> 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)?
Maybe. If I get a chance I want to rethink how we represent aliases in the summary. I think it should be possible to represent them using either a FunctionSummary or GlobalValueSummary built using the aliasee.


https://reviews.llvm.org/D32471