[libcxx-commits] [PATCH] D112152: [libc++] Unroll loop in std::find_if and define other std::find variants in terms of it

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/find.h:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
High-level motivation question: This performance difference that you see, is it //only// at `-O0`? I would hope that the compiler does the obvious loop unrolling at `-O2` at least. I'd also be interested in seeing a Godbolt link of libc++ versus libstdc++, and perhaps even some new benchmark as part of the PR.


================
Comment at: libcxx/include/__algorithm/find.h:29
+  bool operator()(_Up&& __other) const {
+    return __value_ == _VSTD::forward<_Up>(__other);
+  }
----------------
This flips the order from `*first == value` to `value == *first`. I think we're within our rights to do that, technically; but it feels gratuitous.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.find/find_if.pass.cpp:62
 
+int main(int, char**) {
+  test();
----------------
Mordante wrote:
> Since the algorithm is no longer straight forward I think it would be good to validate the number of times the predicate is evaluated. Just to make sure we didn't accidentally increase the number of evaluations.
+1, I think your current implementation has 4 instances of `break` that should be `return __first`, and this causes 1 extra evaluation of `__pred` in most cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112152