[libc-commits] [PATCH] D72516: [llvm-libc] Add memory function benchmarks

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

I did one detailed pass. I don't claim to understand the code fully but in general I do not see anything popping out as a big problem. I left a few inline comments, but an overall comment is that there are style violations at many places. @abrachet pointed out few of them, but I see there are a number of others as well. Especially, the variable naming convention follows google-style and not LLVM-style.



================
Comment at: libc/utils/benchmarks/CMakeLists.txt:56
+
+add_libc_unittest(libc-benchmark-test
+    SRCS libc_benchmark_test.cc
----------------
Would just `add_unittest` work? If it does, it should be used here.


================
Comment at: libc/utils/benchmarks/CMakeLists.txt:92
+add_custom_target(check-libc-benchmark)
+add_dependencies(check-libc-benchmark
+    libc-benchmark-test
----------------
You don't need to do it separately like this. You can specify SUITE option when listing the add_libc_unittest target.


================
Comment at: libc/utils/benchmarks/Memcmp.cpp:34
+    // Copy buffer A to B.
+    memcpy(BBuffer.begin(), ABuffer.begin(), Conf.BufferSize);
+    if (Conf.MemcmpMismatchAt == 0)
----------------
::memcpy ?


================
Comment at: libc/utils/benchmarks/README.md:1
+# Libc mem* benchmarks
+
----------------
Limit to 80 char lines.


================
Comment at: libc/utils/benchmarks/README.md:85
+
+ To learn more about the design decisions behind the benchmarking framework, have a look at the [RATIONALE.md](RATIONALE.md) file.
----------------
RATIONALE.md is missing?


================
Comment at: libc/utils/benchmarks/libc_benchmark.cc:24
+        "CPU scaling is enabled, the benchmark real time measurements may be "
+        "noisy and will incur extra overhead.");
+}
----------------
Can we provide instructions on how to disable CPU scaling? Better, can we provide a script with which one can disable CPU scaling and then restore if they want to?

We don't have to do it with this change, but we should eventually do it. I had to play around a bit to disable CPU scaling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72516