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

[Vedant Kumar via llvm-commits]

vsk added a comment.

I think it'd be fair to add the V2 binary tests after committing, once we're sure the bots are stable. A few more comments --


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

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

================
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.
----------------
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.

================
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);
 
----------------
Where would these names be stripped out? IIUC this vector is in place so we can call `collectPGOFuncNameStrings` in `emitNameData`.


http://reviews.llvm.org/D16388