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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6457
@@ -6454,1 +6456,3 @@
 }
+std::pair<GlobalValue::GUID, CalleeInfo::HotnessType>
+ModuleSummaryIndexBitcodeReader::readCallGraphEdge(
----------------
Blank line above (clang-format should fix?), and document

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:6464
@@ +6463,3 @@
+  GlobalValue::GUID CalleeGUID = getGUIDFromValueId(CalleeValueId).first;
+  if (IsOldProfileFormat)
+    I += 1; // Skip old field - callsite_count.
----------------
I think this block would be clearer if restructured like:

if (IsOldProfileFormat) {
   I += 1; // Skip old callsitecount field
   if (HasProfile)
      I += 1; // Skip old profilecount field
} else
   Hotness = static_cast<CalleeInfo::HotnessType>(Record[++I]);
...

(Note the ++I change in the Record access too)

================
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);
----------------
Not sure I follow. The example was to show how to also create a BFI, so that the PSI is useful. But instead you could follow Mehdi's suggestion and make the PFI argument optional.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:288
@@ -283,2 +287,3 @@
 
-    auto *CalleeSummary = selectCallee(GUID, Threshold, Index);
+    // Fixme: Also lower the threshold for cold callsites.
+    const auto NewThreshold =
----------------
s/Fixme/FIXME/

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:14
@@ +13,3 @@
+
+; HOT-DEFAULT-NOT: define available_externally void @none2()
+; HOT-DEFAULT-NOT: define available_externally void @none3()
----------------
Also check that hot3 not imported?

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:19
@@ +18,3 @@
+
+; Test import with hot multiplier 1.0 - threat hot callsites as normal.
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=1 -import-hot-multiplier=1.0 --S | FileCheck %s --check-prefix=CHECK --check-prefix=HOT-ONE
----------------
s/threat/treat/

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:27
@@ +26,3 @@
+; HOT-ONE-NOT: define available_externally void @none3()
+; HOT-ONE-NOT: define available_externally void @cold2()
+
----------------
hot3?

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:30
@@ +29,3 @@
+
+; Test import with hot multiplier 0.0 and high threshold - don't import functions called form hot callsite.
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=10 -import-hot-multiplier=0.0 --S | FileCheck %s --check-prefix=CHECK --check-prefix=HOT-ZERO
----------------
s/form/from/

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:38
@@ +37,3 @@
+; HOT-ZERO-NOT: define available_externally void @hot2()
+; HOT-ZERO-NOT: define available_externally void @hot1()
+
----------------
hot3?

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:53
@@ +52,3 @@
+  call void @cold2()
+  call void @hot2()
+  call void @none1()
----------------
why is hot2 here?

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:54
@@ +53,3 @@
+  call void @hot2()
+  call void @none1()
+  br label %exit
----------------
ditto for none1


https://reviews.llvm.org/D24638