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

[Piotr Padlewski via llvm-commits]

Prazek added inline comments.

================
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.
----------------
tejohnson wrote:
> 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)
what do you mean with "(Note the ++I change in the Record access too)"
I agree that your structure is better.

================
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);
----------------
tejohnson wrote:
> 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.
I will look into that, but my point is that PSI already have handling of not having the profile data, so when asked if count is hot/cold it returns always false.
changing it to optional will require checking if PSI is set. It won't be big husstle, but I guess it will make it more clean that the PSI does not always 
returns useful information
 

================
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()
----------------
tejohnson wrote:
> Also check that hot3 not imported?
oh yes

================
Comment at: test/Transforms/FunctionImport/hotness_based_import.ll:53
@@ +52,3 @@
+  call void @cold2()
+  call void @hot2()
+  call void @none1()
----------------
tejohnson wrote:
> why is hot2 here?
to check if it takes max of the hotness of block. It is also in hot block.

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


https://reviews.llvm.org/D24638