[libc-commits] [PATCH] D93210: [libc] revamp memory function benchmark

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/benchmarks/LibcMemoryBenchmark.h:41
+  // See 'SweepModeMaxSize' and 'SizeDistributionName' below.
+  bool IsSweepMode = false;
+
----------------
courbet wrote:
> Make this a method ?
> 
> ```
> bool IsSweepMode() {
>   return SweepModeMaxSize > 0;
> }
> ```
I'm not happy with the redundant information either.
That said the structure is shared between C++ and the external world via JSON and I found it simpler to not replicate the logic everywhere and have a simple boolean attribute.


================
Comment at: libc/benchmarks/LibcMemoryBenchmarkMain.cpp:106
+    return [this](ParameterType P) {
+      __llvm_libc::memset(DstBuffer + P.Offset, P.Offset % 0xFF, P.Size);
+      return DstBuffer + P.Offset;
----------------
courbet wrote:
> did you mean `&` ?
Both work but `&` is clearer. Thx for the suggestion.


================
Comment at: libc/benchmarks/LibcMemoryBenchmarkMain.cpp:149
+size_t getL1DataCacheSize() {
+  const std::vector<CacheInfo> CacheInfos = HostState::get().Caches;
+  const auto IsL1DataCache = [](const CacheInfo &CI) {
----------------
courbet wrote:
> why not `const &` ?
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93210