[libcxx-commits] [PATCH] D131395: [libc++] Implement `lexicographical_compare_three_way`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

Removing my "request for changes". Please update the benchmarks as suggested and ship it once @var-const gives you the LGTM.

Thanks!



================
Comment at: libcxx/benchmarks/lexicographical_compare_three_way.bench.cpp:73-79
+  for (auto _ : state) {
+    auto b1 = IteratorT{v1.data()};
+    auto e1 = IteratorT{v1.data() + v1.size()};
+    auto b2 = IteratorT{v2.data()};
+    auto e2 = IteratorT{v2.data() + v2.size()};
+    benchmark::DoNotOptimize(std::lexicographical_compare_three_way(b1, e1, b2, e2, std::compare_three_way()));
+  }
----------------
You seem to be benchmarking not only the call to `std::lexicographical_compare_three_way`, but all of:

```
auto b1 = IteratorT{v1.data()};
auto e1 = IteratorT{v1.data() + v1.size()};
auto b2 = IteratorT{v2.data()};
auto e2 = IteratorT{v2.data() + v2.size()};
benchmark::DoNotOptimize(std::lexicographical_compare_three_way(b1, e1, b2, e2, std::compare_three_way()));
```

I suspect this is where the difference for `random_access_iterator<int*>` vs `int*` comes from. I think it would make sense to instead create the various iterators outside of the timer. And then perhaps regenerate the benchmarks and hopefully the ones for `random_access_iterator<int*>` and `int*` would be within noise.

Also, we did confirm that the assembly was the same for both: https://godbolt.org/z/nTfGcKrzM.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395