[PATCH] D30754: SamplePGO ThinLTO ICP fix for local functions.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM, with some suggestions on comments below.



================
Comment at: include/llvm/ProfileData/SampleProfReader.h:287
+    // The function name may have been updated by adding suffix. In sample
+    // profile, the function names are all stripped, so we need to strip
+    // the function name suffix before matching with profile.
----------------
Are they stripped, or just not yet promoted and renamed?


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3706
+      if (!hasValueId(GUID)) {
+        GUID = Index.getGUIDFromOriginalID(GUID);
+        if (GUID == 0 || !hasValueId(GUID))
----------------
Needs comment about how the call edge may have been added based on indirect call profiles, in which case if it was for SamplePGO it would have used the OriginalID for local functions.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:200
 
+    if (Index.findGlobalValueSummaryList(GUID) == Index.end()) {
+      GUID = Index.getGUIDFromOriginalID(GUID);
----------------
Similar comment required here.


================
Comment at: test/Transforms/PGOProfile/thinlto_samplepgo_icp.ll:6
+
+; Checks if static target functions are properly imported and promoted.
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -o %t4.bc -print-imports 2>&1 | FileCheck %s --check-prefix=IMPORTS
----------------
tejohnson wrote:
> Maybe, "calls to static target functions..." (I assume the promotion you are referring to is the ICP on the call, not the promotion to global of the static function?).
> I assume that the GUID in the value profile metadata is from the original name - add comment to that effect?
This one looks not done yet.


https://reviews.llvm.org/D30754