std.algorithm.cmp with default ordering can use memcmp for all size 1 unsigned types (instead of just char)

[n8sh]

No description provided.

[dlang-bot]

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19412	enhancement	std.algorithm.cmp with default ordering can use memcmp for all size 1 unsigned types (instead of just char)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6776"

[stefan-koch-sociomantic]

you should cache *p1 and *p2
the additional dereference may be expensive

[n8sh]

@stefan-koch-sociomantic I've verified that the compiler performs this optimization without needing any hint from the programmer. Both cmp1 and cmp1 in the example below are compiled to the same machine code by ldc -O2:

https://run.dlang.io/is/cz5hay

import std.algorithm : min;

int cmp1(const dchar[] r1, const dchar[] r2) @trusted
{
    auto p1 = r1.ptr, p2 = r2.ptr, pEnd = p1 + min(r1.length, r2.length);
    for (; p1 != pEnd; ++p1, ++p2)
    {
        if (*p1 != *p2)
            return cast(int)*p1 - cast(int)*p2;
    }
    return int(r1.length > r2.length) - int(r1.length < r2.length);
}

int cmp2(const dchar[] r1, const dchar[] r2) @trusted
{
    auto p1 = r1.ptr, p2 = r2.ptr, pEnd = p1 + min(r1.length, r2.length);
    for (; p1 != pEnd; ++p1, ++p2)
    {
        const v1 = *p1, v2 = *p2; // manually saving result of *p1 and *p2
        if (v1 != v2)
            return cast(int) v1 - cast(int) v2;
    }
    return int(r1.length > r2.length) - int(r1.length < r2.length);
}

void main()
{
}

[thewilsonator]

(Blocked on dscanner - so I stop relooking at this PR)

[radcapricorn]

I'm against this change. We should be removing C dependencies from Phobos, not finding more ways to plug them in. The language should be capable enough without them.

[wilzbach]

I'm against this change. We should be removing C dependencies from Phobos, not finding more ways to plug them in. The language should be capable enough without them.

memcmp is already used in this function.

[radcapricorn]

@wilzbach As I said, "removing, not finding more ways" ;)

[n8sh]

Benchmark of this PR's change:

range	compiler	speedup
ubyte[16]	dmd -O -inline	2.5x
ubyte[128]	dmd -O -inline	10.5x
ubyte[16]	ldc2 -O2	2.4x
ubyte[128]	ldc2 -O2	13.5x

Note: in this benchmark ldc2 produced a 64-bit binary and dmd produced a 32-bit binary.

For wider types whether this was an improvement depended on length, so I've restricted it to just byte-width types.

@radcapricorn instead of using memcmp directly I changed this PR to use core.internal.string.dstrcmp which internally uses memcmp with a separate path for CTFE. Does that satisfy your concerns?

[thewilsonator]

Just address Stefan's comment and I think this is good.