[PATCH] D16388: [PGO] Enable profile name compression

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:32
@@ +31,3 @@
+                                cl::desc("Enable name string compression"),
+                                cl::Hidden, cl::init(true));
+
----------------
vsk wrote:
> IMO this works fine as a non-hidden option, but I'll leave it up to you.
ok will make it non hidden.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:296
@@ -283,3 +295,3 @@
   StringRef ComdatPrefix = (Triple(M.getTargetTriple()).isOSBinFormatCOFF()
-                                ? getInstrProfNameVarPrefix()
+                                ? getInstrProfCountersVarPrefix()
                                 : getInstrProfComdatPrefix());
----------------
vsk wrote:
> This change seems inconsistent with the comment above it -- why start using the counter var prefix for COFF comdats?
Will fix the comment. The name var comdat no long exists.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:372
@@ +371,3 @@
+  // Reset Name variable's linkage and visibility
+  NamePtr->setLinkage(GlobalValue::PrivateLinkage);
+  // Mark the name variable as used so that it isn't stripped out.
----------------
vsk wrote:
> Why is this step required, and how do we know this is the right linkage to reset to? E.g in swift, these name pointers start life with LinkOnceAny linkage.
It is set to privateLinkage so that the name vars can be garbage collected by the compiler. Note that the individual name variable (per function) is created by instrumenter to pass the linkage that can be passed on to the counter and data variable. Once that is done, they are no longer needed. Will update the comment.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:374
@@ -362,1 +373,3 @@
+  // Mark the name variable as used so that it isn't stripped out.
+  ReferencedNames.push_back(NamePtr);
 
----------------
vsk wrote:
> Where would these names be stripped out? IIUC this vector is in place so we can call `collectPGOFuncNameStrings` in `emitNameData`.
This is certainly an out of place comment. Will remove.


http://reviews.llvm.org/D16388