[PATCH] D17212: [ThinLTO] Support for call graph in per-module and combined summary.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:190
@@ -189,3 +189,3 @@
   enum FunctionSummarySymtabCodes {
-    FS_CODE_PERMODULE_ENTRY = 1,  // FS_ENTRY: [valueid, linkage, instcount]
-    FS_CODE_COMBINED_ENTRY  = 2,  // FS_ENTRY: [modid, linkage, instcount]
+    // PERMODULE_NOCALLS: [valueid, linkage, instcount]
+    FS_PERMODULE_NOCALLS = 1,
----------------
davidxl wrote:
> A side note -- it might be useful to reserve some bits for general function attributes such as 'address-taken' etc.
What is the advantage of saving bits now? The format is still highly in flux anyway, and once it is nailed down eventually we would need to use some kind of version id to specify the change regardless of whether it was using saved bits or not.

================
Comment at: include/llvm/IR/FunctionInfo.h:97
@@ +96,3 @@
+
+  /// Record a call graph edge from this function to the function identified
+  /// by \p CalleeGUID, with cumulative profile count (across all calls from
----------------
davidxl wrote:
> Without profile data, it might be useful to record the number of static callsites from a function to the callee.  This can help compiler backend to make better global decisions later (e.g. better inlining and enable more function GC).
I could do that by overloading the count field to hold the static number of callsites. Alternatively, for statically-generated profile information is it useful to record the block frequency sums or something like that?

================
Comment at: include/llvm/IR/FunctionInfo.h:286
@@ +285,3 @@
+  /// or built during the per-module bitcode write process.
+  std::unique_ptr<FunctionSummary> Summary;
+  /// Hold a pointer to the above summary, so that we can still access it after
----------------
davidxl wrote:
> Is this member really needed? The helper is not intended to own the summary.
The helper does own the function summary temporarily. We create it when the function summary section is read, then later transfer ownership to the index when the VST is read. The reason why I need to keep a non-owning pointer to the summary is that later on after all VST entries are read from the combined index I need to set up the call graph edges in the combined index, and for that I need to keep the association here between the function summary and the CallGraphEdgeValueIdList.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5703
@@ +5702,3 @@
+      // callee GUID instead.
+      for (unsigned I = 3, E = Record.size(); I != E; ++I) {
+        BitcodeSummary.addCallGraphEdge(Record[I],
----------------
davidxl wrote:
> Is it better to introduce a symbolic name for the start index instead of hardcoded 3?
I can do that. There's a lot of hardcoded start indices just like this in the file already, but I agree a symbolic name would be clearer.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2520
@@ +2519,3 @@
+/// by the entry block count.
+static uint64_t scaledCount(uint64_t BlockFreq, uint64_t EntryFreq,
+                            uint64_t EntryCount) {
----------------
davidxl wrote:
> Should this be called 'getBlockProfileCount' ?  This utiltiy method belongs to include/llvm/ProfileData/ProfileCommon.h.
Good idea. Will rename and move.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2881
@@ -2778,3 +2880,3 @@
 static void WritePerModuleFunctionSummaryRecord(
-    SmallVector<unsigned, 64> &NameVals, FunctionSummary *FS, unsigned ValueID,
-    unsigned FSAbbrev, BitstreamWriter &Stream) {
+    SmallVector<unsigned, 64> &NameVals,
+    FunctionSummaryIOHelper &BitcodeSummary, unsigned ValueID,
----------------
davidxl wrote:
> Should the type be SmallVector<uint64_t, 64> ? GUID is 64 bit type.
Good point, it should be uint64_t, but for a different reason. We aren't writing the GUID, but rather a value id which is unsigned. But we are writing the profile count when we have profile data, which is uint64_t. Fixing it here and when writing the combined summary.


http://reviews.llvm.org/D17212