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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits