[libcxx-commits] [PATCH] D115125: [libc++] Avoid rejection sampling in `uniform_int_distribution` if possible

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

My initial reaction is that this change slows down the common case to cater to a really super pathological case; I don't think we should do it (but a benchmark could probably change my mind).
The new test needs to move from `libcxx/test/std` to `libcxx/test/libcxx` if it's testing non-mandated, libcxx-specific behavior.



================
Comment at: libcxx/include/__random/uniform_int_distribution.h:244-245
         return static_cast<result_type>(_Eng(__g, _Dt)());
+    else if (_Rg != 0 && _Rg % _Rp == 0)
+        return static_cast<result_type>(__p.a() + (__g() / (_Rg / _Rp)));
     size_t __w = _Dt - __countl_zero(_Rp) - 1;
----------------
(1) `_Rg != 0` is trivially true, right?
(2) Should we be worried that `_Rg % _Rp` is a division by a non-constant value, on the hot path? I think this change needs at least //some// benchmark numbers.
(3) In non-pathological cases, `_Rg` is a power of two, so `_Rg % _Rp == 0` is true only if `_Rp` is also a power of two. But if `_Rp` is a power of two, don't we already do the optimal number of calls to `__g`? So this helps only in the pathological cases where the URBG's range is not a power of two? Perhaps at least we could `if constexpr` (or equivalent) to keep the non-pathological case fast.


================
Comment at: libcxx/test/std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/no_wasted_entropy.pass.cpp:41
+
+// Returns numbers in the range [0, 24) which always have the lowest two bits set.
+struct non_uniform_rbg {
----------------
I'm pretty sure this is library UB.
http://eel.is/c++draft/rand.req.urng#1 "A uniform random bit generator g of type G is a function object returning unsigned integer values such that **each value in the range of possible results has (ideally) equal probability** of being returned."

If this test case is deliberately doing weird stuff to test a corner case, then OK (but it needs to be more explicit about what corner case it's testing).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115125