[PATCH] D16388: [PGO] Enable profile name compression
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 26 15:37:03 PST 2016
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 @@
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())
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 @@
+ NamesVal, "__llvm_prf_nms");
For consistency, could you add a getter for "__llvm_prf_nms" to InstrProf.h?
More information about the llvm-commits