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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Still LGTM except the new comments don't seem quite right to me.



================
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.
----------------
danielcdh wrote:
> tejohnson wrote:
> > Are they stripped, or just not yet promoted and renamed?
> They should be all stripped. As discussed offline, if they are not stripped, we need to have a reliable way to map the unstripped name back to IR, which is undesirable.
Ok, right.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3707
+        // For SamplePGO, the call targets for indirect local functions will
+        // have its name stripped thus GUID is the original. We try to find
+        // corresponding GUID from the Original ID for the matching.
----------------
This is not the name stripping in the profile issue though. This is because the SamplePGO doesn't use the PGOFuncName scheme of computing the GUID based on "SourceFileName:FunctionName" for locals, right? 


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:202
+      // For SamplePGO, the call targets for indirect local functions will
+      // have its name stripped thus GUID is the original. We try to find
+      // corresponding GUID from the Original ID for the matching.
----------------
Ditto


https://reviews.llvm.org/D30754