On Saturday, 26 November 2016 at 16:31:40 UTC, Ilya Yaroshenko wrote:
> 1. Improve RNG generation performance by making code more friendly for CPU pipelining. Tempering (finalization) operations was mixed with internal payload update operations.

A note on this.  The `opCall` (or, in the range version, `popFront`) of Ilya's implementation mixes together two superficially independent actions:

  (1) calculating the current random variate from the current index
      of the internal state array;

  (2) updating the current index of the internal state array, and
      moving to the next entry.

It's straightforward to split out these two procedures into two separate methods (or at least two clearly separated sequences within the `opCall`), but doing so results in a notable performance hit (on my machine, something in the order of 1 GB/s less random bits).

Intertwining these steps in this way is therefore a very smart optimization (although TBH it feels a little worrying that it's necessary).

On Wednesday, 14 December 2016 at 00:18:06 UTC, Joseph Rushton Wakeling wrote:
> Cool.  Last question: IIUC you use the private `_z` parameter as a cache for the most recent `data[index]` value (and AFAICT that's the only use it has).  Is there a good reason for doing this, rather than just setting `z = data[index]` inside `opCall` (where `z` is the local parameter inside the method)?

Because memory access locality. z is always hot.

On Saturday, 26 November 2016 at 20:13:36 UTC, Andrei Alexandrescu wrote:
> On 11/26/16 11:31 AM, Ilya Yaroshenko wrote:
>> Hey,
>>
>> 32-bit Mt19937 random number Generator is default in Phobos.
>> It is default in Mir too, except that 64-bit targets use 64-bit Mt19937 instead.
>
> Congrats! Also thanks for using the Boost license which would allow backporting the improvements to Phobos. Who'd be up for it?

PR is now on Phobos, and in the process of looking at this together, Ilya and I resolved a number of issues both in this code and in mir.random:
https://github.com/dlang/phobos/pull/5011

There is unfortunately one serious blocker to this PR.  In order to allow it to be a drop-in replacement for the previous Phobos implementation, I had to find a means to default-initialize its internal state at compile time so that it would match what would be there if it was explicitly seeded with the default seed.

The method I developed works fine with LDC, but fails with DMD: the internal state of the generator winds up as all zeros, except for the `State.z` parameter which mysteriously ends up at the correct value.  This would suggest that somehow the generated state is being overwritten _after_ being correctly generated, rather than not correctly generated in the first place.

Any chance that someone with more CTFE-fu than myself could take a glance and advise what might be wrong (or how to work around it)?

On 1/7/17 6:16 PM, Joseph Rushton Wakeling wrote:
>
> The method I developed works fine with LDC, but fails with DMD: the
> internal state of the generator winds up as all zeros, except for the
> `State.z` parameter which mysteriously ends up at the correct value.
> This would suggest that somehow the generated state is being overwritten
> _after_ being correctly generated, rather than not correctly generated
> in the first place.
>
> Any chance that someone with more CTFE-fu than myself could take a
> glance and advise what might be wrong (or how to work around it)?

This indicates a compiler bug in dmd itself, so the best outcome for this library work would be a reduced compiler bug + a simple library workaround. -- Andrei

On Sunday, 8 January 2017 at 02:51:51 UTC, Andrei Alexandrescu wrote:
> This indicates a compiler bug in dmd itself, so the best outcome for this library work would be a reduced compiler bug + a simple library workaround. -- Andrei

Yes, that much is clear -- sorry if I wasn't clear enough myself.  I'm asking for eyes on the problem because reducing it to a minimal example appears non-trivial, while the bug itself looks serious beyond its effect on this PR.

For what it's worth:

  * it appears to be a backend issue, not frontend (using dmd with the same
    frontend as the latest ldc doesn't make a difference);

  * creating an `enum` manifest constant from the CTFE function works fine,
    but assigning to the runtime struct field from that enum produces the
    same mostly-zeros result;

  * it definitely doesn't appear to be something as trivial as just default
    initializing a struct instance (or a struct instance including a static
    array of more than a certain size) being the problem.

I will keep trying to reduce it to a minimal example, but CTFE-related backend bugs are not something I have a lot of experience with tracking down, hence the request for help.

Would filing an issue still be helpful at this stage, even without a minimal example?

On Sunday, 8 January 2017 at 13:16:29 UTC, Joseph Rushton Wakeling wrote:
> I'm asking for eyes on the problem because reducing it to a minimal example appears non-trivial, while the bug itself looks serious beyond its effect on this PR.

I underestimated myself :-P  Minimal example is as follows:

/////////////////////////////////////////////////////////////////
struct Inner
{
    uint value = void;  // <=== removing this `void` initializer
                        // removes the problem
}

struct Outer
{
    Inner inner = defaultInner();

    static Inner defaultInner()
    {
        if (!__ctfe) assert(false);
        Inner inn;
        inn.value = 23;
        return inn;
    }
}

void main()
{
    import std.stdio : writeln;
    Outer outer;
    outer.inner.writeln;  // outputs `Inner(0)` with dmd
                          // but should be `Inner(23)`
                          // ldc gets it right ;-)
}
/////////////////////////////////////////////////////////////////

So, the bug comes down to priority of default initialization.  I'll follow an issue later today and fix the PR accordingly.