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

[David Li via llvm-commits]

davidxl 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,
----------------
A side note -- it might be useful to reserve some bits for general function attributes such as 'address-taken' etc.

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

================
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
----------------
Is this member really needed? The helper is not intended to own the summary.

================
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],
----------------
Is it better to introduce a symbolic name for the start index instead of hardcoded 3?

================
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) {
----------------
Should this be called 'getBlockProfileCount' ?  This utiltiy method belongs to include/llvm/ProfileData/ProfileCommon.h.

================
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,
----------------
Should the type be SmallVector<uint64_t, 64> ? GUID is 64 bit type.


http://reviews.llvm.org/D17212