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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 18 inline comments as done.
tejohnson added a comment.

I think I've addressed all the comments.



================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:48
+// Size of memory mapped to a single shadow location.
+static const uint64_t DefaultShadowGranularity = 64;
+
----------------
MaskRay wrote:
> 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`)
changed all of them.


================
Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:203
+
+class HeapProfilerLegacyPass : public FunctionPass {
+public:
----------------
MaskRay wrote:
> 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.
Since the legacy pass manager is still the default, and adding the support was trivial, it makes sense to add the support there too.


================
Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:61
+; Exactly one shadow update for store access.
+; CHECK-COUNT-1: %[[NEW_ST_SHADOW:[^ ]*]] = add i64 %{{.*}}, 1
+; CHECK-COUNT-1: store i64 %[[NEW_ST_SHADOW]]
----------------
MaskRay wrote:
> `CHECK-COUNT-1` here is actually identical to a `CHECK`. If there are two repeated lines, the pattern will still match.
> 
> Suggest deleting `-COUNT-1`. Change the next `CHECK` to a `CHECK-NEXT: store x86_fp80 0xK3FFF8000000000000000, x86_fp80* %a`
That doesn't work as that store x86_64 (the original store) is not NEXT. I just want to make sure that we have one shadow store, which was where the COUNT-1 came from. I've change that to a regular CHECK and added a CHECK-NOT store i64 before and after that sequence. Ditto elsewhere.



================
Comment at: llvm/test/Instrumentation/HeapProfiling/instrumentation-use-callbacks.ll:16
+; CHECK-CALL-COUNT-4: call void @__heapprof_load
+; CHECK-CALL-NOT: call void @__heapprof_load
+; CHECK-CUSTOM-PREFIX-COUNT-4: call void @__foo_load
----------------
MaskRay wrote:
> With appropriate `-NEXT:` patterns, `-NOT` is not really needed.
> 
> It is also important to test the argument passed to `__heapprof_load`
The -NOT is needed to ensure there are no additional calls, without matching the full IR. I've expanded the testing to test the arguments though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85948