[PATCH] Random Number Generator Refactoring (removing from Module)

[JF Bastien]

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:36
@@ +35,3 @@
+  /// the pass name) provided by the client pass. Each pass which
+  /// needs randomness should instantiate its own pass, using a unique
+  /// seed. This ensures a reproducible random stream even when other
----------------
I think you mean "instantiate its own RNG".

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:37
@@ +36,3 @@
+  /// needs randomness should instantiate its own pass, using a unique
+  /// seed. This ensures a reproducible random stream even when other
+  /// randomness consuming passes are added or removed.
----------------
The pass uses a unique salt, not seed.

I'd also clarify that the random stream is per-pass, which means that different source versions of LLVM are likely to perform the same per-pass random choices.

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:39
@@ -40,1 +38,3 @@
+  /// randomness consuming passes are added or removed.
+  RandomNumberGenerator(const Module &M, StringRef PassSalt);
 
----------------
As Alp pointed out this should probably not know about Module. You'll probably want some factory that knows about Module, Pass and RNG, and instantiate RNGs with salt, and make this class non-constructible otherwise.

What do you have in mind for the Pass salt? They usually have a Pass name that could be expected to stay the same, as well as a Pass argument (in PassInfo).

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:42
@@ -41,3 +41,3 @@
   /// Returns a random number in the range [0, Max).
-  uint64_t next(uint64_t Max);
+  uint_fast64_t operator()();
 
----------------
Why the change to uint_fast64_t? Seems odd with the rest of the codebase not using it, but I guess there's no harm either.

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:47
@@ -46,1 +46,3 @@
   // http://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine
+  // This RNG should be deterministicly portable across C++11
+  // implementations.
----------------
I'd drop the "should", since it's spec'd to generate the same random sequence when the seed is the same.

s/deterministicly/deterministically/

================
Comment at: lib/Support/RandomNumberGenerator.cpp:41
@@ +40,3 @@
+  // might be problematic if the input filename extension changes
+  // (e.g. from .c to .bc or .ll).
+  //
----------------
I'm not sure that's actually a problem.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:53
@@ +52,3 @@
+  // Note: std::seed_seq can only store 32-bit values, even though we
+  // are using a 64-bit RNG. This isn't a problem since the Mersenn
+  // twister constructor copies these correctly into its initial state
----------------
Mersenne

http://reviews.llvm.org/D4377