fix Issue 20990 - Optimizer should move cold branches to the end of t…

[WalterBright]

…he function

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20990	enhancement	Optimizer should move cold branches to the end of the function

Testing this PR locally

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

dub run digger -- build "master + dmd#11374"

[thewilsonator]

The test doesn't actually test anything

[WalterBright]

The test doesn't actually test anything

What it does is tickle the optimization, so if it crashes the compiler it'll show up.

[Geod24]

What it does is tickle the optimization, so if it crashes the compiler it'll show up.

But then, I'm sure we already have plenty of code in the testsuite that already does this.
Any test that would pass before the patch doesn't make sense to me.

[WalterBright]

I'm sure we already have plenty of code in the testsuite that already does this.

Maybe we do, maybe we don't. I like having a specific test for it.

Any test that would pass before the patch doesn't make sense to me.

There are many optimizer tests like that in the test suite.

[WalterBright]

The failing Win64 autotest seems to be related to dlang/druntime#2436

[WalterBright]

Which @rainers has just fixed!

[ibuclaw]

There are many optimizer tests like that in the test suite.

There has been some discussions of adding in a directive to scan the resultant assembler to ensure it produces what we expect it to, but there's not really any strong desire to add it.

[ibuclaw]

To my dismay, there's not even the word "cold" anywhere in the patch, so can't pretend I know what is going on here.

Checked with the following:

class Foo
{
    int *ptr;
    int inc;
}

class Bar
{
    Foo foo;
    int inc;
}

class Baz
{
    Bar bar;
    int inc;
}

// foo() and bar() should produce the same code when
// optimized.

void foo(Baz baz)
{
    if (baz is null)
        assert(false);
    baz.inc++;
    if (baz.bar is null)
        assert(false);
    baz.bar.inc++;
    if (baz.bar.foo is null)
        assert(false);
    baz.bar.foo.inc++;
    if (baz.bar.foo.ptr is null)
        assert(false);
    *baz.bar.foo.ptr = 42;
}

void bar(Baz baz)
{
    assert(baz);
    baz.inc++;
    assert(baz.bar);
    baz.bar.inc++;
    assert(baz.bar.foo);
    baz.bar.foo.inc++;
    assert(baz.bar.foo.ptr);
    *baz.bar.foo.ptr = 42;
}

And the output is what I expect it to be, so seems reasonable to me.

[ibuclaw]

However it doesn't cater for early returns instead of asserts.

int foo(Baz baz)
{
    if (baz is null)
        return 0x10;
    baz.inc++;
    if (baz.bar is null)
        return 0x20;
    baz.bar.inc++;
    if (baz.bar.foo is null)
        return 0x40;
    baz.bar.foo.inc++;
    if (baz.bar.foo.ptr is null)
        return 0x80;
    *baz.bar.foo.ptr = 42;
    return 0x100;
}

Though the above may be more reliant on branch prediction, or understanding the interdependencies between branches in order to determine what is likely or unlikely to happen.

[WalterBright]

However it doesn't cater for early returns instead of asserts.

The optimizer is built on the presumption that the hot branch of if-then-else is the then branch.