[libcxx-commits] [PATCH] D125329: Replace modulus operations in std::seed_seq::generate with conditional checks.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

> Abseil benchmarks suggest that the conditional checks result in faster code (4-5x)
> as they can be compiled into conditional move instructions.

Nice improvement! Did state that the cmov is the big win?

In general quite happy with the changes, but I really like a bit more comment explaining the cleverness.



================
Comment at: libcxx/benchmarks/random.bench.cpp:20
+static void BM_SeedSeq_Generate(benchmark::State& state) {
+  std::array<std::uint32_t, MAX_BUFFER_LEN> buffer;
+  std::array<std::uint32_t, MAX_SEED_LEN> seeds;
----------------
philnik wrote:
> laramiel wrote:
> > Does the benchmark need to support C++03? because std::array
> No. We normally don't have any divergence of performance critical code between C++ versions, so we only run the benchmarks for C++20(?).
Correct the benchmarks are indeed only tested with C++20.


================
Comment at: libcxx/include/__random/seed_seq.h:88
 void
 seed_seq::generate(_RandomAccessIterator __first, _RandomAccessIterator __last)
 {
----------------
I saw `libcxx/test/std/numerics/rand/rand.util/rand.util.seedseq/generate.pass.cpp` tests the generated output of this function.


================
Comment at: libcxx/include/__random/seed_seq.h:112
         }
+        size_t __kmodn = 0;          // __k % __n
+        size_t __kpmodn = __p % __n; // (__k + __p) % __n
----------------
This is a kind of clever code which isn't really obvious. Can you add a bit more comment what it exactly does. After you did that the `__k % __n` comment becomes clear.


================
Comment at: libcxx/include/__random/seed_seq.h:151
         }
         for (size_t __k = __m; __k < __m + __n; ++__k)
         {
----------------
I think here we can add some comment too. The modulo in this loop works when `__k == __m`, which is true when `__s + 1 <= __m`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125329