Issue 13419 - Remove comma expression in std.uni

[schuetzm]

This is a patch by Ketmar Dark:
https://issues.dlang.org/show_bug.cgi?id=13419

[quickfur]

LGTM

[DmitryOlshansky]

Yeah, thanks guys... Bored yawn.
Now it would be awesome if any of you have an idea how to bring inlining of these functions back (in DMD) without comma expression. :)

[schuetzm]

Yes, something really needs to be done about this annoying restriction. But it needs to be done in DMD. We should probably put more non-inlineable code into Phobos, to raise the pressure ;-)

[DmitryOlshansky]

@schuetzm I agree. The problem is it's users who would loose on performance, while we shuffle work between compiler and phobos teams.

So ideally it's first fix the compiler, then rewrite code. But alas compiler guys would say that since everybody in the know already writes code using comma to get inlining, it's not a top priority. Here we go catch-22.

[yebblies]

So ideally it's first fix the compiler, then rewrite code.

Agreed. I'm a little surprised to hear it doesn't inline this already.

But alas compiler guys would say that since everybody in the know already writes code using comma to get inlining, it's not a top priority. Here we go catch-22.

We would?

[DmitryOlshansky]

But alas compiler guys would say that since everybody in the know already writes code using comma to get inlining, it's not a top priority. Here we go catch-22.

We would?

@yebblies Well, okay maybe they won't :) I wish to be proven wrong, it come up before, not a single time. There seems to always be some pressing matters far greater then inlining of basic if statements and multiple early returns. It especially hurts functions that return something.

[quickfur]

DMD's inlining is really poor. It gives up too easily. I always find that gdc's inlining is far superior.

[mihails-strasuns]

I am marking this as blocked until inlining issue is fixed in DMD.

[yebblies]

Looking at the first function, this can be inlined

char truncate()(char ch) pure @safe
{
    ch -= 0x80;
   return ch < 0x40 ? ch : (badEncoding(), cast(char)0);
}

But this can't:

char truncate()(char ch) pure @safe
{
    ch -= 0x80;
    if (ch < 0x40) return ch;
    badEncoding();
    return cast(char)0);
}

But this one can:

char truncate()(char ch) pure @safe
{
    ch -= 0x80;
    if (ch < 0x40)
        return ch;
    else
    {
        badEncoding();
        return cast(char)0);
    }
}

ie The if-and-fallthough means the inliner's primitive flow analysis can't tell that function 2 always returns and can therefore be turned into an expression.
To detect this, the inliner needs a) a canonicalization pass to generate if-else blocks everywhere, b) a more sophisticated return-detection algorithm or c) to be moved into the backend.

@DmitryOlshansky Of the other functions modified in this pull, which ones are actually prevented from being inlined?

[DmitryOlshansky]

@yebblies I'll try to use the if/else wokraround and see if there any other patterns that can't be re-written or problematic.

[quickfur]

I still think the inliner should be improved. But at least that shouldn't block this PR anymore.

[quickfur]

@schuetzm Please update code to if/else branch to ensure inlining in dmd.

[schuetzm]

I changed all except the one in Impl.lookup, where the else block would encompass the entire remaining function.

[quickfur]

Looks reasonable; Impl.lookup is a huge function anyway and I don't think anyone would expect it to be inlined. LGTM, I'll wait a bit for other reviewers (esp. @DmitryOlshansky ) to take a look before merging.

[mihails-strasuns]

Yep, it is up to Dmitry to call it merge-ready

[DmitryOlshansky]

Aye-aye. I'll just run my benchmarks with and without this patch. Somewhat busy at the moment, please nag me if I forget.

[DmitryOlshansky]

Seems like all is in order.
https://gist.github.com/DmitryOlshansky/7ce9394ccc5bf27359da

[DmitryOlshansky]

Auto-merge toggled on

[quickfur]

Stumbled across the bugzilla issue related to the if/else branch inlining issue today; thought I should log it here (i dunno, for posterity, or perhaps somebody will take a look at it): https://issues.dlang.org/show_bug.cgi?id=7625