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
