[libc] Change rand implementation so all tests pass in both 32- and 64-bit systems

[mikhailramalho]

The current xorshift star algorithm fails to produce uniform enough values in 32-bit systems, so this patch changes the algorithm so the tests now pass in both 32- and 64-bit systems.

The multiplier used in the algorithm was obtained from "The Art of Computer Programming" book. It is also used by musl and newlib.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Mikhail R. Gadelha (mikhailramalho)

Changes

The current xorshift star algorithm fails to produce uniform enough values in 32-bit systems, so this patch changes the algorithm so the tests now pass in both 32- and 64-bit systems.

The multiplier used in the algorithm was obtained from "The Art of Computer Programming" book. It is also used by musl and newlib.

---

Full diff: https://github.com/llvm/llvm-project/pull/98692.diff

1 Files Affected:

• (modified) libc/src/stdlib/rand.cpp (+8-8)

diff --git a/libc/src/stdlib/rand.cpp b/libc/src/stdlib/rand.cpp
index ff3875c2f6959..a38d3af7b8aaa 100644
--- a/libc/src/stdlib/rand.cpp
+++ b/libc/src/stdlib/rand.cpp
@@ -13,18 +13,18 @@
 
 namespace LIBC_NAMESPACE {
 
-// An implementation of the xorshift64star pseudo random number generator. This
-// is a good general purpose generator for most non-cryptographics applications.
+// This multiplier was obtained from Knuth, D.E., "The Art of
+// Computer Programming," Vol 2, Seminumerical Algorithms, Third
+// Edition, Addison-Wesley, 1998, p. 106 (line 26) & p. 108 */
 LLVM_LIBC_FUNCTION(int, rand, (void)) {
   unsigned long orig = rand_next.load(cpp::MemoryOrder::RELAXED);
   for (;;) {
-    unsigned long x = orig;
-    x ^= x >> 12;
-    x ^= x << 25;
-    x ^= x >> 27;
-    if (rand_next.compare_exchange_strong(orig, x, cpp::MemoryOrder::ACQUIRE,
+    uint64_t x = orig;
+    x = static_cast<unsigned long>(6364136223846793005ULL) * x;
+    if (rand_next.compare_exchange_strong(orig, static_cast<unsigned long>(x),
+                                          cpp::MemoryOrder::ACQUIRE,
                                           cpp::MemoryOrder::RELAXED))
-      return static_cast<int>((x * 0x2545F4914F6CDD1Dul) >> 32) & RAND_MAX;
+      return static_cast<int>(x >> 32) & RAND_MAX;
     sleep_briefly();
   }
 }

[jhuber6]

If we change x to uint64_t wouldn't that also solve the issue? It's called xorshift64star because it needs 64-bit state, but I forgot that unsigned long was 32-bit on some systems.

[mikhailramalho]

I've tried, and a static_cast back to unsigned long on compare exchange call, but the fsb test still failed

…

On Fri, 12 Jul 2024, 17:49 Joseph Huber, ***@***.***> wrote: ***@***.**** commented on this pull request. If we change x to uint64_t wouldn't that also solve the issue? It's called xorshift64star because it needs 64-bit state, but I forgot that unsigned long was 32-bit on some systems. — Reply to this email directly, view it on GitHub <#98692 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKEJHZ45JOGB6OW3ICRTNLZMA6MDAVCNFSM6AAAAABKZRQQ6SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZVG4ZDIOBYGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[SchrodingerZhu]

Can you resolve conflicts?

[michaelrj-google]

LGTM

[jhuber6]

I still think these should just be separate functions, the constants aren't arbitrary and can't be mixed, making it a template is just confusing.

[mikhailramalho]

I still think these should just be separate functions, the constants aren't arbitrary and can't be mixed, making it a template is just confusing.

I can move the code to rand()'s body, wdyt?

LLVM_LIBC_FUNCTION(int, rand, (void)) {
  unsigned long orig = rand_next.load(cpp::MemoryOrder::RELAXED);

  if constexpr (sizeof(void *) == sizeof(uint64_t)) {
    for (;;) {
      unsigned long x = orig;
      x ^= x >> 12;
      x ^= x << 25;
      x ^= x >> 27;
      if (rand_next.compare_exchange_strong(orig, x, cpp::MemoryOrder::ACQUIRE,
                                            cpp::MemoryOrder::RELAXED))
        return static_cast<int>((x * 0x2545F4914F6CDD1Dul) >> 32) & RAND_MAX;
      sleep_briefly();
    }
  } else {
    for (;;) {
      unsigned long x = orig;
      x ^= x >> 13;
      x ^= x << 27;
      x ^= x >> 5;
      if (rand_next.compare_exchange_strong(orig, x, cpp::MemoryOrder::ACQUIRE,
                                            cpp::MemoryOrder::RELAXED))
        return static_cast<int>(x * 1597334677ul) & RAND_MAX;
      sleep_briefly();
    }
  }
  __builtin_unreachable();
}

[jhuber6]

I still think these should just be separate functions, the constants aren't arbitrary and can't be mixed, making it a template is just confusing.

I can move the code to rand()'s body, wdyt?

LLVM_LIBC_FUNCTION(int, rand, (void)) {
  unsigned long orig = rand_next.load(cpp::MemoryOrder::RELAXED);

  if constexpr (sizeof(void *) == sizeof(uint64_t)) {
    for (;;) {
      unsigned long x = orig;
      x ^= x >> 12;
      x ^= x << 25;
      x ^= x >> 27;
      if (rand_next.compare_exchange_strong(orig, x, cpp::MemoryOrder::ACQUIRE,
                                            cpp::MemoryOrder::RELAXED))
        return static_cast<int>((x * 0x2545F4914F6CDD1Dul) >> 32) & RAND_MAX;
      sleep_briefly();
    }
  } else {
    for (;;) {
      unsigned long x = orig;
      x ^= x >> 13;
      x ^= x << 27;
      x ^= x >> 5;
      if (rand_next.compare_exchange_strong(orig, x, cpp::MemoryOrder::ACQUIRE,
                                            cpp::MemoryOrder::RELAXED))
        return static_cast<int>(x * 1597334677ul) & RAND_MAX;
      sleep_briefly();
    }
  }
  __builtin_unreachable();
}

Works for me, though having the function name helps people know what it's supposed to be. If it's an internal function it'll just be trvially dropped if uncalled.