[PATCH] D64082: [MemFunctions] Add microbenchmarks for memory functions.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Looks ok to me from `benchmark` perspective, but some more thoughts about the benchmark itself..



================
Comment at: MicroBenchmarks/MemFunctions/main.cpp:43
+  benchmark::DoNotOptimize(q_storage);
+
+  for (auto _ : state) {
----------------
courbet wrote:
> lebedev.ri wrote:
> > This may be paranoia, but i'm not sure this is sufficient to guarantee that compiler *can't* just look into `p`/`q`.
> > I'd suggest adding this here:
> > ```
> >   benchmark::DoNotOptimize(p);
> >   benchmark::DoNotOptimize(q);
> > 
> >   benchmark::ClobberMemory(p);
> >   benchmark::ClobberMemory(q);
> > ```
> > (i see that you do that for `std::vector<char>`'s already, but you have already acquired `_storage.data()`..)
> Sounds reasonable. I've even moved the ClobberMemory inside the call (and verified that benchmark numbers do not change).
Nice.


================
Comment at: MicroBenchmarks/MemFunctions/main.cpp:50
+    for (int i = 0; i < kNumElements; ++i) {
+      int res = Pred()(memcmp(p + i * kSize, q + i * kSize, kSize));
+      benchmark::DoNotOptimize(res);
----------------
I think i'm forgetting about some magic.
All the predicates (`EqZero`, ...) take a single argument, how does this work if it passes two args?


================
Comment at: MicroBenchmarks/MemFunctions/main.cpp:58-66
+struct EqZero {
+  bool operator()(int v) const { return v == 0; }
+};
+struct LessThanZero {
+  bool operator()(int v) const { return v < 0; }
+};
+struct GreaterThanZero {
----------------
To be noted, none of these is the actual `memcmp`, i think?


================
Comment at: MicroBenchmarks/MemFunctions/main.cpp:59
+struct EqZero {
+  bool operator()(int v) const { return v == 0; }
+};
----------------
Does it matter that these take `int` while you always pass `char`?


Repository:
  rT test-suite

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

https://reviews.llvm.org/D64082