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

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thanks, this is awesome!

1. I've noticed that some fields (e.g `NameRef`) are typed as `uint64_t` and as `IntPtrT` somewhat interchangeably. Wdyt of adopting a convention where `IntPtrT` is reserved for values which have already been byte-swapped?

2. There are some binary tests in `test/tools/llvm-cov/Inputs/` for version 1 of the coverage mapping format. Could you add similar tests for version 2?


================
Comment at: include/llvm/ProfileData/InstrProfData.inc:164
@@ -162,1 +163,3 @@
+	           llvm::IndexedInstrProf::ComputeHash(NameValue)))
+#endif
 COVMAP_FUNC_RECORD(const uint32_t, llvm::Type::getInt32Ty(Ctx), DataSize, \
----------------
I think an updated to CoverageMappingFormat.rst belongs with this commit. This should be a fairly small change (mostly lines removed).

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:375
@@ +374,3 @@
+  if (ReferencedNames.empty())
+    return;
+
----------------
If we don't create `NamesVar` is there no name section, and if so, is that ever a problem?

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:385
@@ +384,3 @@
+                                      llvm::GlobalValue::PrivateLinkage,
+                                      NamesVal, "__llvm_prf_nms");
+  NamesVar->setSection(getNameSection());
----------------
For consistency, could you add a getter for "__llvm_prf_nms" to InstrProf.h?


http://reviews.llvm.org/D16388