[PATCH] D42212: [ThinLTO] Add call edges' relative block frequency to per-module summary.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:259
   FS_CFI_FUNCTION_DECLS = 18,
+  // PERMODULE_RELBF: [valueid, flags, instcount, numrefs,
+  //                   numrefs x valueid,
----------------
Could use a description of when this is used.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:58
   HotnessType Hotness = HotnessType::Unknown;
+  uint64_t RelBlockFreq = 0;
 
----------------
Do we need 64 bits in practice? Especially since these are scaled down by the entry freq. Concerned about the size increase for large indices that have huge numbers of edges. Also, since we'll never have both the Hotness and the RelBlockFreq, it would be great if we could union them, although that would require a flag on the index and I suppose we might want to support mixing modules built with and without profile data? Perhaps some other way to combine them. I.e. use a single field and reserve the lowest 3 bits for the hotness, shifting the relative frequency into the upper n-3 bits...


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:63
+cl::opt<bool> WriteRelBFToSummary(
+    "write-relbf-to-summary", cl::Hidden, cl::init(false),
+    cl::desc("Write relative block frequency to function summary "));
----------------
Suggest moving this to the bitcode writer and always adding RelBlockFreq into the in-memory index here.

Is there any reason to default off? Why not just default on from the get-go?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:290
+          // being rounded down to 0.
+          BBFreq /= BFI->getEntryFreq();
+          CallGraphEdges[Index.getOrInsertValueInfo(
----------------
Any chance getEntryFreq will be 0?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:292
+          CallGraphEdges[Index.getOrInsertValueInfo(
+                             cast<GlobalValue>(CalledValue))]
+              .updateRelBlockFreq(BBFreq);
----------------
Suggest saving the ValueInfo already created earlier on line 280.


Repository:
  rL LLVM

https://reviews.llvm.org/D42212