Instead of using the C++-linker, use the D compiler to link LDC2 and LDMD

[JohanEngelen]

This removes the need for the CMake logic to figure out what linker flags to pass the C++linker to link D code (50 lines of flaky cmake script).

Continuation from #1361

[dnadlinger]

Could you just mention DMD/Windows here explicitly?

[dnadlinger]

Thanks for continuing this rather cumbersome job!

[JohanEngelen]

I need to beat this thing

[JohanEngelen]

I am not sure if it was a good idea to compile all D source in one go. It makes compilation significantly slower when you just change one cpp file...

[dnadlinger]

when you just change one cpp file

If you are worried about .cpp files, we can always split up the C and D parts of the compilation process and use DMD just for linking (in case you are not doing this already).

Regarding .d files, DMD compiles all at once and it seems to work out fine for them.

[JohanEngelen]

Regarding .d files, DMD compiles all at once and it seems to work out fine for them.

Yes, but it's just a little slower when only one file changed.
So perhaps in future, someone may want to spend some time to split D compilation from the linking step again.

[dnadlinger]

"or driver/ldmd.cpp?"

[dnadlinger]

Thanks!

[JohanEngelen]

I wonder if compiling all D source at once improves performance when built with LDC. LDC becomes much faster (tested on Weka source) when built with dmd --inline, but ldmd2 --inline results in an LDC that is about 20% slower.

[dnadlinger]

ldmd2 --inline doesn't even do anything. Sorry, I misremembered that. Apparently we do pass -enable-inlining to LDC, but it shouldn't affect the inliner when -O2/… is also given.

[dnadlinger]

I wonder if compiling all D source at once improves performance when built with LDC.

Be sure to also use -singleobj (which is the default for LDMD when compiling to an executable) when doing that, so we can actually do cross-module inlining, etc.

[JohanEngelen]

Green on AppVeyor and Travis.
Does this PR also work on everybody's build setup?

[JohanEngelen]

Don't merge yet.

make install does not mark ldc2 and ldmd2 as executables. Working on a fix.

[JohanEngelen]

Results from a "quick" test on some code that takes a while to compile (CMAKE_BUILD_TYPE=Release):
pre-PR: 91 sec
post-PR: 77 sec
Boom!

[JohanEngelen]

Install of executables is fixed.

Please merge!

[dnadlinger]

Let's do this! It's definitely a step forward, and we better figure out any potentially remaining issues as soon as possible.

[JohanEngelen]

Hmm the Windows x86 build fails, because it cannot find a pdb file. For x86, DMD uses its own linker which probably cannot generate pdb's?

[kinke]

Hmm, I thought DMD supports PDB, but maybe only if the full source is in D, and not for COFF objects produced by the MSVC++ compiler. Anyway, I only include the PDB to enable easier troubleshooting for people experimenting with CI builds. We can also just omit it for the x86 build if we don't find a better solution.

Edit: The x86 jobs actually fail in std.math, the missing PDB would be ignored anyway. I bet the DMD linker assumes 80-bit reals.

[JohanEngelen]

@kinke Could be a missing flag?

Edit: good that you checked the log again.

[JohanEngelen]

I am going to try and reproduce this on Windows, by following the appveyor.yml file for 32bit compilation.

[JohanEngelen]

This PR fixes issue #1379

[kinke]

@klickverbot: merging this just killed our 32-bit MSVC build. Please don't discard AppVeyor issues.

[kinke]

I'll look into a fix. The issue seems to be std.math.lrint() only.

Edit: std.math.lrint(int.max - 0.5) != 2147483646L (it's 2147483648 instead). No idea why this fails (the 3 other test cases do work) when using DMD to link LDC's 32-bit COFF objects. Best guess is a bug in DMD's linker.

[kinke]

This snippet demonstrates the problem:

enum MANTISSA_LSB = 0;
enum MANTISSA_MSB = 1;

long lrint(real x)
{
    long result;

    // Rounding limit when casting from real(double) to ulong.
    enum real OF = 4.50359962737049600000E15L;

    uint* vi = cast(uint*)(&x);

    // Find the exponent and sign
    uint msb = vi[MANTISSA_MSB];
    uint lsb = vi[MANTISSA_LSB];
    int exp = ((msb >> 20) & 0x7ff) - 0x3ff;
    int sign = msb >> 31;
    msb &= 0xfffff;
    msb |= 0x100000;

    if (exp < 63)
    {
        if (exp >= 52)
            result = (cast(long) msb << (exp - 20)) | (lsb << (exp - 52));
        else
        {
            // Adjust x and check result.
            real j = sign ? -OF : OF;
            x = (j + x) - j;
            msb = vi[MANTISSA_MSB];
            lsb = vi[MANTISSA_LSB];
            exp = ((msb >> 20) & 0x7ff) - 0x3ff;
            msb &= 0xfffff;
            msb |= 0x100000;

            if (exp < 0)
                result = 0;
            else if (exp < 20)
                result = cast(long) msb >> (20 - exp);
            else if (exp == 20)
                result = cast(long) msb;
            else
                result = (cast(long) msb << (exp - 20)) | (lsb >> (52 - exp));
        }
    }
    else
    {
        // It is left implementation defined when the number is too large.
        return cast(long) x;
    }

    return sign ? -result : result;
}

void main()
{
    import core.stdc.stdio;
    printf("actual = %lld, expected = %lld\n", lrint(int.max - 0.5), 2147483646L);
    printf("actual = %lld, expected = %lld\n", lrint(int.max + 0.5), 2147483648L);
    printf("actual = %lld, expected = %lld\n", lrint(int.min - 0.5), -2147483648L);
    printf("actual = %lld, expected = %lld\n", lrint(int.min + 0.5), -2147483648L);
}

=>

actual = 2147483648, expected = 2147483646
actual = 2147483648, expected = 2147483648
actual = -2147483648, expected = -2147483648
actual = -2147483648, expected = -2147483648

The output is correct if lrint() takes a double argument instead of a 64-bit real.

[kinke]

I've just compared the IR for above lrint(real) vs. lrint(double). The result is very interesting: the constant int.max - 0.5 results in different double-precision IR constants: 0x41DFFFFFFFE00000 (64-bit real) vs. 0x41DFFFFFFFA00000 (double). This is most likely related to @JohanEngelen's recent Phobos workaround which also involved floating-point literals and mixed types...

[dnadlinger]

Sorry, I had missed the AppVeyor failures; feel free to revert.

[dnadlinger]

I can't make heads or tails of the failure, though. Linking is done using MSVC link.exe in both cases. Thinking about it, it's more likely that this is due to the frontend now being built in all-at-once mode, exposing more cross-module inlining possibilities.

[kinke]

RealExp::toElem: 2.14748e+09L @ real
* RealExp::toConstElem: 2.14748e+09L @ real | 0x1.fffffffe00000p+30

vs.

RealExp::toElem: 2.14748e+09 @ double
* RealExp::toConstElem: 2.14748e+09 @ double | 0x1.fffffffa00000p+30

Apparently DtoType() doesn't return a LLVM double-precision type in this case for 64-bit real, and the 64-bit compile-time real is loaded as 80-bit x87 value to initialize the LLVM constant, or something like that...

[dnadlinger]

I seem to be a bit on the slow side today (hence also the merge in the first place), but do we have a clue yet as to why this happens? Is this due to -inline?

[kinke]

No idea. Maybe we just got lucky and found a lurking bug which hasn't shown up until now. Sadly with that DMD-linked build, I don't have any PDBs so I cannot debug what's going on in LDC. I'll continue investigating.

[JohanEngelen]

@kinke So this PR means we lose a lot of debugging convenience on Windows? :(
I think LDC still cannot build itself on Windows, so we could really use the debugging capability...

[kinke]

Okay so it's not DtoConstFP() - the front-end's compile-time real stored in RealExp already diverges. So the front-end somehow computes int.max - 0.5 differently if it's desired type is a 64-bit real (2147483647.5!) and not double (2147483646.5, correct). At least when linking LDC with whatever DMD is using for 32-bit COFF objects.

[kinke]

Oh wait - when using DMD to compile the front-end, the front-end uses 80-bit compile-time reals. LDC's glue layer for MSVC targets only uses 64-bit reals. I'd bet we've missed a conversion somewhere...

Edit: It's really time for a proper real_t. ;)

[JohanEngelen]

At least when linking LDC with whatever DMD is using for 32-bit COFF objects.

When DMD builds the executable, is there any difference between -m32 and -m32mscoff ?

[kinke]

I'm 99.9% sure that this is because we miss most and not just a few ddmd real/d_float80 (80-bit when compiling with DMD, 64-bit with LDC) vs. LDC C++ longdouble (64-bit for MSVC builds) conversions.
Unfortunately, fixing it would require the first commit of #1317, a backport of dlang/dmd#5471. So we'll have to wait for it to get merged. I told you I had a bad feeling when using DMD to compile the front-end for MSVC builds. ;)

[dnadlinger]

But the AppVeyor build was always using DMD, right?

[dnadlinger]

When DMD builds the executable, is there any difference between -m32 and -m32mscoff ?

Sure; OMF vs. COFF and optlink vs. MSVC.

[kinke]

I'm also convinced this is the reason why the is operator didn't work to compare compile-time reals for ARM in Port.fequal() (@smolt - the front-end probably memcmp()ing 10 bytes instead of 8).
It simply all boils down to requiring a unified real_t across the D front-end and the C++ glue layer.

But the AppVeyor build was always using DMD, right?

Yep, LDC 0.17 crashes when building our current front-end. ;)
Not sure why this hasn't hit us before and why it's currently no problem for 64-bit MSVC. I've just rebased #1317. Its first commit needs some testing, it could/should get rid of the issue. Maybe it's then also able to compile the front-end without crashing.

[kinke]

Its first commit needs some testing, it could/should get rid of the issue.

Nope, same issue. :/

[kinke]

... but db9af1d (built with DMD) is at least able to build the front-end without crashing, and the resulting LDC is able to build druntime+Phobos (debug+release). Tested with 32-bit MSVC and by manually adding -L/NODEFAULTLIB:libvcruntime.lib for the ldmd2.exe/ldc2.exe Ninja targets (linked with ldmd2.exe - LDC and LDMD themselves link against the dynamic MSVC runtime due to CMake defaults).

[kinke]

All commits of #1317 (built with DMD) gets rid of this issue, i.e., int.max - 0.5 for 64-bit target real (represented by llvm::APFloat based software compile-time real_t with double-precision semantics) now finally gives 2147483646.5 (32-bit MSVC).

[smolt]

Does this PR also work on everybody's build setup?

It seems that runtime is not rebuilt if ldc2 was rebuild. Did a dependency on ldc2 get dropped?

[smolt]

I'm also convinced this is the reason why the is operator didn't work to compare compile-time reals for ARM in Port.fequal() (@smolt - the front-end probably memcmp()ing 10 bytes instead of 8).

I think that was a different, mysterious reason. The is operator worked fine for ARM 64-bit real. The failure was on X86 hosts (e.g. OS X) when LDC was compiled by DMD. LDC compiled by LDC was fine to.

[JohanEngelen]

It seems that runtime is not rebuilt if ldc2 was rebuild. Did a dependency on ldc2 get dropped?

Noticed it too. I checked the dependencies, and they look correct but obviously aren't.

Edit: working on a cmake fix now.
Edit: Fixed in master.

[smolt]

Probably totally unrelated but while building LDC master today on Pi to make sure this PR works in that environment, hit a malloc assertion during unittest build:

[ 81%] Generating std/range/package-unittest.o                                  
ldc2: malloc.c:2905: __libc_malloc: Assertion `!victim || ((((mchunkptr)((char*\
)(victim) - 2*(sizeof(size_t)))))->size & 0x2) || ar_ptr == (((((mchunkptr)((ch\
ar*)(victim) - 2*(sizeof(size_t)))))->size & 0x4) ? ((heap_info *) ((unsigned l\
ong) (((mchunkptr)((char*)(victim) - 2*(sizeof(size_t))))) & ~((2 * (512 * 1024\
)) - 1)))->ar_ptr : &main_arena)' failed.
Aborted

I don't have core dump enabled so nothing to point to. Malloc assertions often point to memory corruption. Nothing to do now, just leaving behind a clue.

Second time worked.