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