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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

LGTM with some nits



================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:124
+
+namespace {
+
----------------
maybe remove all static about and extend namespace {}


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:136
+
+static ShadowMapping getShadowMapping(Triple &TargetTriple, int LongSize) {
+  ShadowMapping Mapping;
----------------
this could be ShadowMapping::ShadowMapping()


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:136
+
+static ShadowMapping getShadowMapping(Triple &TargetTriple, int LongSize) {
+  ShadowMapping Mapping;
----------------
vitalybuka wrote:
> this could be ShadowMapping::ShadowMapping()
params are not used?


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:151-155
+  if (TargetTriple.isOSEmscripten()) {
+    return kHeapProfEmscriptenCtorAndDtorPriority;
+  } else {
+    return kHeapProfCtorAndDtorPriority;
+  }
----------------
For consistency
or even "?:"


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:194
+
+private:
+  void initializeCallbacks(Module &M);
----------------
private/public  can be reordered and struct converted to class


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:386
+    auto *F = CI->getCalledFunction();
+    if (F && (F->getName().startswith("llvm.masked.load.") ||
+              F->getName().startswith("llvm.masked.store."))) {
----------------
F->getIntrinsicID() == Intrinsic::masked_load
F->getIntrinsicID() == Intrinsic::masked_store


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:614
+    for (auto &Inst : BB) {
+      if (isInterestingMemoryAccess(&Inst, &IsWrite, &TypeSize, &Alignment) ||
+          isa<MemIntrinsic>(Inst))
----------------
probably nicer to put output param into struct {} and rely on RVO
as is it's hard to track if they are initialized


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85948