[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:148
 CODEGENOPT(MergeFunctions    , 1, 0) ///< Set when -fmerge-functions is enabled.
+CODEGENOPT(HeapProf    	     , 1, 0) ///< Set when -fmemprof is enabled.
 CODEGENOPT(MSVolatile        , 1, 0) ///< Set when /volatile:ms is enabled.
----------------
There is a tab on this line. This makes the code not indented in `git diff` output.


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:45
+
+#define LLVM_HEAP_PROFILER_VERSION 1
+
----------------
Consider `constexpr int LLVM_HEAP_PROFILER_VERSION = 1;`


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:48
+// Size of memory mapped to a single shadow location.
+static const uint64_t DefaultShadowGranularity = 64;
+
----------------
For a constant in the source file, consider `constexpr uint64_t DefaultShadowGranularity = 64;` (constexpr implies const, which means internal linkage in a namespace scope, so you can avoid `static`)


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:57
+static const uint64_t HeapProfEmscriptenCtorAndDtorPriority = 50;
+static const char *const HeapProfInitName = "__heapprof_init";
+static const char *const HeapProfVersionCheckNamePrefix =
----------------
`constexpr char HeapProfInitName[] = "__heapprof_init";`


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:100
+
+static cl::opt<int> ClMappingScale("heapprof-mapping-scale",
+                                   cl::desc("scale of heapprof shadow mapping"),
----------------
Consider deleting `DefaultShadowScale`.

Alternatively, `cl::init(DefaultShadowScale)` and don't check getNumOccurrences() below.


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:105
+static cl::opt<int>
+    ClMappingGranularity("heapprof-mapping-granularity",
+                         cl::desc("granularity of heapprof shadow mapping"),
----------------
ditto


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:146
+
+uint64_t getCtorAndDtorPriority(Triple &TargetTriple) {
+  return TargetTriple.isOSEmscripten() ? HeapProfEmscriptenCtorAndDtorPriority
----------------
Add static. 

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:159
+
+/// HeapProfiler: instrument the code in module to profile heap accesses.
+class HeapProfiler {
----------------
Don’t duplicate function or class name at the beginning of the comment.

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:203
+
+class HeapProfilerLegacyPass : public FunctionPass {
+public:
----------------
Since this is new. Perhaps not deal with legacy pass manager at all?

For example, recently there has been objection on porting CGProfile to the legacy pass manager. For an entirely new thing, not handling legacy pass managers can avoid many tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85948/new/

https://reviews.llvm.org/D85948