This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Support int8_t and uint8_t in integer distributions as an extension
ClosedPublic

Authored by ldionne on Jun 1 2022, 1:28 PM.

Details

Summary

In D125283, we ensured that integer distributions would not compile when
used with arbitrary unsupported types. This effectively enforced what
the Standard mentions here: http://eel.is/c++draft/rand#req.genl-1.5.

However, this also had the effect of breaking some users that were
using integer distributions with unsupported types like int8_t. Since we
already support using __int128_t in those distributions, it is reasonable
to also support smaller types like int8_t and its unsigned variant. This
commit implements that, adds tests and documents the extension. Note that
we voluntarily don't add support for instantiating these distributions
with bool and char, since those are not integer types. However, it is
trivial to replace uses of these random distributions on char using int8_t.

It is also interesting to note that in the process of adding tests
for smaller types, I discovered that our distributions sometimes don't
provide as faithful a distribution when instantiated with smaller types,
so I had to relax a couple of tests.

Supersedes D125283.

Diff Detail

Event Timeline

ldionne created this revision.Jun 1 2022, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 1:28 PM
ldionne requested review of this revision.Jun 1 2022, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 1:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Hi Louis,

This patch should resolve >95% of the issues we're encountering in ChromeOS and Android. I think the remaining char ones we can fix on our end, as the vast majority of the breakages are with uint8_t in particular.
We appreciate the fix very much, as it's been quite a trek on our end patching up these issues (and we're still encountering them, so we'll cherrypick this ASAP). I know the Android folks will be very grateful as well for the fix.

Thanks,
~Jordan R AW

ldionne retitled this revision from [libc++] Support char and unsigned char in integer distributions as an extension to [libc++] Support int8_t and uint8_t in integer distributions as an extension.Jun 2 2022, 5:44 AM
ldionne updated this revision to Diff 433749.Jun 2 2022, 7:40 AM

Fix documentation build. I think this is going to fail in two of the test's distributions because we apparently struggle to be as faithful for small types.

After a small investigation, I think this is because our poisson distribution isn't as good for types with fewer bits, but we'll have to figure that out before shipping the patch.

  • Either we're OK with the distribution not being as faithful, and we relax the tests, or
  • we find a way to improve the implementation for smaller types.
ldionne updated this revision to Diff 446497.Jul 21 2022, 7:33 AM

Rebase, try to fix CI (this was much more involved than I thought).

In particular, I left a bunch of TODOs around the quality of negative_binomial (which is really on poisson_distribution,
since that's what negative_binomial uses). I am out of time for figuring out how to further improve poisson_distribution
for small types. I tried various things and nothing really worked for int8_t -- I think a rewrite of the algorithm might
be necessary, but that would be an ABI break and a larger project.

ldionne updated this revision to Diff 446619.Jul 21 2022, 1:37 PM

Try to fix tests. Do not revert modifications to poisson_distribution anymore, since that causes the reproducer tests I had added to hang.

ldionne updated this revision to Diff 446653.Jul 21 2022, 3:59 PM

Add missing test_macros.h include

ldionne accepted this revision as: Restricted Project.Jul 22 2022, 5:31 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 22 2022, 5:33 AM
This revision was automatically updated to reflect the committed changes.