[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:60
+
+  while (++__first != __last) {
+    _Iter __i = __first;
----------------
ldionne wrote:
> philnik wrote:
> > ldionne wrote:
> > > Per our discussion: it would be interesting to check which of the following options is best in terms of codegen:
> > > 
> > > 1. Use `[[unlikely]]` on the `if (++__first == __last)` branch to tell the compiler that we rarely hit the end of the range, or
> > > 2. If we have random-access iterators, handle the case of an odd-sized range explicitly above so we can get rid of the `if (++__first == __last)` check in the loop altogether and always process two elements at a time, or
> > > 3. Leave the code as-is
> > https://quick-bench.com/q/ezc6hrE4rQ4Smt-XXF72ENAx3iI
> > It looks like using `[[unlikely]]` makes no difference and checking the size before the loop is slightly slower.
> Alright, in that case I don't mind leaving the code as-is, but thanks for checking. Would you mind adding your simple benchmark code to `libcxx/benchmarks/algorithms.bench.cpp`?
I added the benchmark, but I think I'm doing something wrong there. The numbers don't look correct:
```
BM_MinMaxElement_uint32_Random_1            1.15 ns         1.14 ns    612630528
BM_MinMaxElement_uint32_Random_4            1.81 ns         1.80 ns    387973120
BM_MinMaxElement_uint32_Random_16           1.85 ns         1.84 ns    380370944
BM_MinMaxElement_uint32_Random_64           1.06 ns         1.06 ns    659816448
BM_MinMaxElement_uint32_Random_256         0.661 ns        0.660 ns   1000079360
BM_MinMaxElement_uint32_Random_1024        0.487 ns        0.486 ns   1000079360
BM_MinMaxElement_uint32_Random_16384       0.431 ns        0.431 ns   1000079360
BM_MinMaxElement_uint32_Random_262144      0.422 ns        0.421 ns   1000079360
```
Only the numbers for `string` look normal:
```
BM_MinMaxElement_string_Random_1            2.36 ns         2.35 ns    299368448
BM_MinMaxElement_string_Random_4            16.7 ns         16.6 ns     42205184
BM_MinMaxElement_string_Random_16           24.6 ns         24.6 ns     28835840
BM_MinMaxElement_string_Random_64           24.4 ns         24.4 ns     29097984
BM_MinMaxElement_string_Random_256          24.4 ns         24.3 ns     29097984
BM_MinMaxElement_string_Random_1024         28.0 ns         27.9 ns     25165824
BM_MinMaxElement_string_Random_16384        31.2 ns         31.1 ns     22806528
BM_MinMaxElement_string_Random_262144       36.2 ns         36.1 ns     20709376

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637