[PATCH] D24638: [thinlto] Basic thinlto fdo heuristic

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks!

I haven't gone through your new test cases yet, but there are a few comments below so far (one correctness issue with handling old versions).


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:66
@@ -64,2 +65,3 @@
 }
-
+static CalleeInfo::HotnessType getHotness(uint64_t ProfileCount,
+                                          ProfileSummaryInfo &PSI) {
----------------
Add blank line before

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6220
@@ -6219,3 +6219,3 @@
   const uint64_t Version = Record[0];
-  if (Version != 1)
-    return error("Invalid summary version " + Twine(Version) + ", 1 expected");
+  const bool OldVersion = Version == 1;
+  if (!OldVersion && Version != 2)
----------------
Use something like OldProfileFormat to be more explicit. That way the name of this variable won't need changing when we bump the version again.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6303
@@ +6302,3 @@
+        if (OldVersion)
+          I += 2; // Skip 2 old fields (callsite_count, profile_count).
+        else if (HasProfile) // can interpret hotness with new version.
----------------
Only skipping 2 fields if HasProfile, otherwise just skip 1 (callsite_count). Can you make sure there is a test that has the old format bitcode (both with and without old profile)? I.e. you would commit old bitcode - there are some existing tests that do this (look at the .bc files committed in test/Bitcode for examples).

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6362
@@ -6357,3 +6361,3 @@
     // FS_COMBINED: [valueid, modid, flags, instcount, numrefs,
-    //               numrefs x valueid, n x (valueid, callsitecount)]
+    //               numrefs x valueid, n x (valueid)]
     // FS_COMBINED_PROFILE: [valueid, modid, flags, instcount, numrefs,
----------------
Record descriptions in include/llvm/Bitcode/LLVMBitCodes.h need to be updated accordingly.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6393
@@ +6392,3 @@
+        if (OldVersion)
+          I += 2; // Skip 2 old fields (callsite_count, profile_count).
+        else if (HasProfile) // Can interpret hotness with new version.
----------------
Ditto here.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:382
@@ -381,1 +381,3 @@
+      ProfileSummaryInfo PSI(TheModule);
+      auto Index = buildModuleSummaryIndex(TheModule, nullptr, PSI);
       WriteBitcodeToFile(&TheModule, OS, true, &Index);
----------------
The PSI is not too useful without a BFI - perhaps construct one here as we do e.g. in PartialInlinerImpl::unswitchFunction? Ok as a follow-on though. Otherwise change PSI to be passed as an optional pointer.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:287
@@ -282,3 +286,3 @@
     }
-
-    auto *CalleeSummary = selectCallee(GUID, Threshold, Index);
+    const auto NewThreshold =
+        Edge.second.Hotness == CalleeInfo::HotnessType::Hot
----------------
Add a FIXME about using the Code hotness to reduce threshold


https://reviews.llvm.org/D24638