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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I am still trying to read the logic. Some comments to the tests. Some comments are applicable to other tests.



================
Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:22
+; CHECK:   %[[LOAD_ADDR:[^ ]*]] = ptrtoint i32* %a to i64
+; CHECK:   %[[MASKED_ADDR:[^ ]*]] = and i64 %[[LOAD_ADDR]], -64
+; CHECK-S3:   %[[SHIFTED_ADDR:[^ ]*]] = lshr i64 %[[MASKED_ADDR]], 3
----------------
It is important to add some `-NEXT:` here for better FileCheck diagnostics when things change.


================
Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:23
+; CHECK:   %[[MASKED_ADDR:[^ ]*]] = and i64 %[[LOAD_ADDR]], -64
+; CHECK-S3:   %[[SHIFTED_ADDR:[^ ]*]] = lshr i64 %[[MASKED_ADDR]], 3
+; CHECK-S5:   %[[SHIFTED_ADDR:[^ ]*]] = lshr i64 %[[MASKED_ADDR]], 5
----------------
`CHECK-S3-NEXT` can follow `CHECK-NEXT`


================
Comment at: llvm/test/Instrumentation/HeapProfiling/basic.ll:54
+
+define void @LongDoubleTest(x86_fp80* nocapture %a) nounwind uwtable {
+entry:
----------------
FP80Test may be more suitable.

(-mlong-double-128 can change the width)


================
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]]
----------------
`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`


================
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
----------------
With appropriate `-NEXT:` patterns, `-NOT` is not really needed.

It is also important to test the argument passed to `__heapprof_load`


================
Comment at: llvm/test/Instrumentation/HeapProfiling/scale-granularity.ll:13
+}
+; CHECK-GRAN-LABEL: @read_granularity
+; CHECK-GRAN-NOT: ret
----------------
(Optional) One style I try to do to improve readability: when `-LABEL` or `-NEXT` is used, indent CHECK lines more than the `-LABEL`

```
; CHECK-GRAN-LABEL: @read_granularity(
; CHECK-GRAN:         and {{.*}} -32
; CHECK-GRAN-NEXT:    lshr {{.*}} 3
```


================
Comment at: llvm/test/Instrumentation/HeapProfiling/scale-granularity.ll:19
+
+define i32 @read_scale(i32* %a) {
+entry:
----------------
The IR of these functions is the same. Consider using one function with different sets of CHECK lines.


================
Comment at: llvm/test/Instrumentation/HeapProfiling/version-mismatch-check.ll:4
+
+; RUN: opt < %s -heapprof-module                                        -S | FileCheck %s
+; RUN: opt < %s -heapprof-module -heapprof-guard-against-version-mismatch=0 -S | FileCheck %s --check-prefix=NOGUARD
----------------
`|` is misaligned. Consider not aligning at all


================
Comment at: llvm/test/Instrumentation/HeapProfiling/version-mismatch-check.ll:11
+; CHECK-LABEL: define internal void @heapprof.module_ctor()
+; CHECK:         call void @__heapprof_version_mismatch_check_
+; NOGUARD-NOT:   call void @__heapprof_version_mismatch_check_
----------------
`call void @__heapprof_version_mismatch_check_v1()`

Otherwise there is no test checking the exact version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85948