Compile compiler-rt with MSVC

[retep998]

Compiles just the minimal subset of compiler-rt needed for MSVC and it does so using MSVC.

Depends on rust-lang/compiler-rt#10

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[huonw]

I suspect this will either optimise less well or be less precise (or both), so maybe it makes sense to have a cfg to only use this form on MSVC?

[retep998]

The compiler-rt implementation is pretty similar, so it should optimize just as well unless the platform has a native instruction for it. I can cfg it out to only be used on msvc though.

[huonw]

I would personally prefer this as the following, which is simpler (IMO):

while b != 0 {
     if b % 2 != 0 { r *= a }
     b /= 2;
     a *= a;
}
if n < 0 { r.recip() } else { r }

But I don't feel particularly strongly. (Yes, I know the code as written does an additional a *= a.)

If you feel strongly about avoiding that extra a *= a I even prefer this form (i.e. basically just avoid folding the return into a complicated expression in the loop):

loop {
     if b % 2 != 0 { r *= a }
     b /= 2;
     if b == 0 { break }
     a *= a;
}
if n < 0 { r.recip() } else { r }

[huonw]

it should optimize just as well unless the platform has a native instruction for it

I'm talking about optimising at a higher level than just codegen, e.g. constant folding/reasoning about variable ranges etc.

Also, if it's a similar implementation, that means my precision concern isn't a concern.

[retep998]

Okay, I switched to using cfg so that the pure Rust version is only used for msvc.

[angelsl]

Perhaps we should do something like rlibc and continue to emit the opcodes, but instead of linking to compiler-rt/builtins we provide our own so that LLVM can do whatever optimisations it does (?).

compiler-rt/builtins provides the things listed here but we really only need to implement those that we emit/we allow LLVM to emit.

Of course the other option is to get compiler-rt/builtins to build on MSVC, but that is going to be hell because it uses parts of C99 that MSVC doesn't support.

[angelsl]

Yeah, this works on our current nightlies. You can verify that compiler-rt isn't being depended on by replacing it with an empty compiler-rt.lib file (i.e. type NUL > empty.c, cl /c /Tcempty.c, lib empty.obj).

pub fn main() {
    println!("{}", 2.0f32.powi(4));
}

#[no_mangle]
pub extern fn __powisf2(mut a: f32, mut b: i32) -> f32
{
    let recip = b < 0;
    let mut r = 1f32;
    loop {
        if b & 1 == 1 { r *= a; }
        b /= 2;
        if b == 0 { break; }
        a *= a;
    }
    if recip { 1f32/r } else { r }
}

#[no_mangle]
pub extern fn __powidf2(mut a: f64, mut b: i32) -> f64
{
    let recip = b < 0;
    let mut r = 1f64;
    loop {
        if b & 1 == 1 { r *= a; }
        b /= 2;
        if b == 0 { break; }
        a *= a;
    }
    if recip { 1f64/r } else { r }
}

Edit: Of course if we do this we lose the ability to just inline the whole thing which happens if we simply don't emit the opcode.

[angelsl]

Is there a compiler flag to set no_compiler_rt? Because if we simply disable compiler_rt from being built immediately then build fails because our stage0 compiler expects compiler_rt to exist.

Otherwise the solution is to apply this for one nightly and then actually stop building it the next.

[retep998]

@angelsl We'd have to wait till the next snapshot, not nightly, to disable compiler-rt from building.

[angelsl]

@retep998

We'd have to wait till the next snapshot, not nightly, to disable compiler-rt from building.

Bah. I'll just put in my hackfix then.

[retep998]

I just did a simple test of checking the emitted asm and constant folding seems entirely unaffected, the inline constants in the asm are bit for bit identical.
https://gist.github.com/retep998/969089222a8ac84646ac

[alexcrichton]

Unfortunately I don't think we can take this step just yet. We need a guaranteed way to inform LLVM that it can't lower anything to a call to compiler-rt, otherwise it's likely to pop up from time to time and we'll have to go patch up various implementations here and there as they arise.

For example, a 32-bit target requires the __mulodi4 intrinsic for a function like:

fn foo(a: i64, b: i64) -> i64 {
    a.checked_mul(b).unwrap_or(0)
}

It looks like other commonly used intrinsics from compiler-rt, such as for operations like u64 % u64 on 32-bit, are wired up to functions that are found in the CRT, so we're covered on some fronts, but not all.

I agree it'd be nice to not have to build compiler-rt on MSVC, but I don't think it's worth "just hoping" that LLVM doesn't start lowering to any functions in compiler-rt. Another example of that currently LLVM doesn't lower expressions like A * A * A * A * A * A to a call to the powi intrinsics, but it would be nice to do so. In that case we'd have to tell LLVM that the intrinsic simply isn't available rather than just not calling it ourselves.

[retep998]

Perhaps instead of avoiding using the intrinsics, we should just write pure Rust implementations of the intrinsics themselves?

Considering you can use Clang with MSVC without compiler-rt, it should be entirely feasible for Rust to do so as well.

[alexcrichton]

Perhaps instead of avoiding using the intrinsics, we should just write pure Rust implementations of the intrinsics themselves?

Unfortunately even that isn't enough to cut it unless we start linking as all the weird intrinsic names (which they themselves are subject to change). LLVM frequently lowers to new intrinsics or creates instructions later lowered to new intrinsics as the result of optimizations, so we need a way to tell LLVM that it cannot perform these optimizations or lowerings by letting it know the intrinsic functions are not available.

Considering you can use Clang with MSVC without compiler-rt, it should be entirely feasible for Rust to do so as well.

This is good news to know! It should help in learning what they do so we may be able to mirror it. Taking pot shots at the compiler, however, saying "X supports Y, why doesn't Rust" may not be so productive.

[bors]

☔ The latest upstream changes (presumably #28668) made this pull request unmergeable. Please resolve the merge conflicts.

[retep998]

With regards to __mulodi4, Clay was having similar issues jckarter/clay#164. The equivalent msvc intrinsic is _mulx_u64 which isn't defined for 32-bit targets, and since C/C++ has no way to do multiplication with overflow (aside from those intrinsics) neither Clang nor msvc have to deal with this, so there's not much in the way of prior art in this area.

I doubt LLVM will ever start doing optimizations that lower to intrinsics which msvc doesn't provide since Clang is supposed to work without compiler-rt in the msvc toolchain. Thus it should be enough for us to simply not emit intrinsics which don't have a native instruction or msvc function and just have our own implementation instead.

[retep998]

Apparently D faced a similar issue and falls back to implementing multiplication with overflow themselves. https://github.com/ldc-developers/druntime/blob/c5a1f3186ffe7e0bcbbcb291d9d0df9c7d668614/src/core/checkedint.d#L584-L600

[alexcrichton]

Interesting! I wonder if perhaps if the LLVM folks have much to say about this? LLVM seems to claim that generated code that it should work without compiler-rt, but that seems contradictory given the current situation of how some intrinsics are lowered. It may be worth asking them if there's a precise set of intrinsics that are expected to not work or if there's a set which are intended to only work, and perhaps we can work with that?

Inlining implementations where necessary seems fine for now, and we may also want to have them with #[cfg(target_env = "msvc")] to be safe, but I'm mostly just concerned about the set of intrinsics we need to guard against.

[angelsl]

I think LLVM assumes the intrinsics are there, there is a way to set some intrinsics names to nullptr which seems to stop them from being emitted (?) but it doesn't seem to apply to all intrinsics. I'll dig through LLVM more when I find time..

I believe if we simply do not emit any llvm.* intrinsics we won't get any calls to compiler-rt (only to e.g. memset etc which for us now are in MSVCRT)

Someone on LLVM IRC yesterday said that "in a freestanding environment there are a subset of intrinsics that the user is expected to provide" and that it's documented in GCC, .. but I couldn't find anything. He might have misunderstood. Seems like this part of LLVM is pretty unknown :/

[retep998]

I decided to instead take the route of just compiling the necessary bits of compiler-rt with MSVC. Only one small change was needed to compiler-rt due to not needing to compile most of it. The changes to the make stuff are rather hairy and I don't really know if they're correct or robust. I hate make and I hope it gets replaced with something much easier to understand.

[angelsl]

Turns out we can't actually tell LLVM to not emit calls to __powi*.

Here you can see that some intrinsics that are not always available are simply set to nullptr. But only uses of those intrinsics check that they are available (e.g. here) i.e. not nullptr — trying to do this for those we are concerned about i.e. __powi* results in LLVM segfaulting.

[angelsl]

OK, I got compiler-rt builtins compiling on MSVC.

Trying to upstream it: http://reviews.llvm.org/D13384

[alexcrichton]

Holy cow, nice work @angelsl! @retep998 would you be ok holding out to see the resolution of whether that gets merged upstream? It sounds like that'd be the ideal case!

[angelsl]

Should be merged soon, once I resolve some outstanding issues: link.

[angelsl]

Reviewer is merging the changes. They should be in tomorrow.

[alexcrichton]

Nice, thanks @angelsl! @retep998 this'll probably now want to start using cmake to build compiler-rt (we're already doing so for LLVM on MSVC), so it's probably fine to start basing this off a compiler-rt branch with those patches merged.

[angelsl]

Ah, one thing about cmake..

The cmake compiler-rt build expects either an already-compiled LLVM with the path to llvm-config passed to cmake, otherwise it should be located in the LLVM source tree in projects/ and configured and built with LLVM.

So if we're compiling rustc with a prebuilt LLVM there is no problem, we just give the path to llvm-config.

But if we have to compile LLVM also we have to either build LLVM and then call cmake on compiler-rt later on with the path to the built LLVM, or put the compiler-rt submodule into llvm/projects/ so it is built with LLVM.

Someone told me git supports having a submodule within the directory of another submodule (with both submodules of the same parent project) but I haven't tried that out.

The other thing is having all this MSVC-specific code so that we use cmake to build LLVM on MSVC and automake otherwise, but ... seems like LLVM is shifting towards using cmake wherever possible (including on Linux, etc) — this is an entirely separate issue though.

[angelsl]

Update: I forgot about the intrinsics written in assembly. LLVM's assembler files are in AT&T syntax while MASM only takes Intel notation ... MSBuild silently ignored the assembler files (!) so I didn't account for them until LLVM's buildbots which build with Ninja instead of MSBuild failed.

Now I can either choose to port the assembly files to Intel syntax which GAS will accept, or use the C implementations for the functions. Problem with the former is that that would mean LLVM has two different syntaxes for assembly in its source tree which isn't very nice.

I'm looking into using the C implementations when building with MSVC. The functions that are affected aren't actually used by Rust at the moment which is why when I tried the builtins library I built with those functions missing it still worked.

Alternatively, someone from LLVM said that they're working towards lowering libcalls to the MSVCRT instead. In that case we can continue using MinGW to build builtins until that works.

[angelsl]

All the changes except actually enabling the build on MSVC have been committed into the LLVM tree.

You can test it by changing this line to simply else ().

There are some builtins for which the code output from C is pretty dismal compared to the handcrafted assembly output. Rust does not use any of them, however.

[alexcrichton]

Awesome, thanks @angelsl! @retep998 do you want to update this PR to use cmake to build compiler-rt as well?

[angelsl]

00:23:40 <%Retep9981> angelsl: I don't want to touch make
00:23:45 <%Retep9981> angelsl: make is scary

---

Rust currently builds LLVM on Windows by invoking CMake in configure and then starting the build later on when we actually run make. We also happen to treat compiler-rt separately from LLVM.

compiler-rt can be built as either standalone or with LLVM. When it is built with LLVM it should be located under projects/ in the LLVM folder, and CMake will configure it along with the rest of LLVM when CMake is invoked for LLVM. When it is built standalone it expects LLVM to already be compiled and it requires the path to llvm-config passed to it when CMake is invoked on it.

So if we do not move compiler-rt we cannot call CMake for it during configure because we don't have LLVM yet, and calling CMake during actual make seems quite terrible. The other way is to move src/compiler-rt into src/llvm/projects... do we have a Git submodule for that path (so we have two, one for src/compiler-rt and src/llvm/projects/compiler-rt — yes, apparently Git supports this) (this means we have to update both submodules, bad) or do we move the directory (creating problems when Git is used later on because now its submodule is missing) or do we create a symlink (which we cannot do from Windows because that needs administrative rights (yay Windows) nor can we do from MSYS2 because that just creates a special file that the MSYS2 layer translates into a symlink for MSYS2 programs but Windows programs (e.g. MSBuild) won't know how to deal with)?

Yeah, this sucks. There's no good solution.

[alexcrichton]

I think it's fine to call cmake as part of the normal build process, this is rarely build and builds quickly so it's not really any skin of anyone's back.

[angelsl]

@alexcrichton Oh, OK then.

Will do it the next few days.