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

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 3 15:43:34 PST 2016

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


More information about the llvm-commits mailing list