Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Floating point add gives different results than compiler-rt on arm-unknown-linux-gnueabi #90

Closed
mattico opened this issue Oct 3, 2016 · 20 comments · Fixed by #155
Closed

Comments

@mattico
Copy link
Contributor

mattico commented Oct 3, 2016

I'm still trying to figure out what was wrong with #48, and I'd like another pair of eyes on this because I have no idea what is going on.

I added an implementation of arbitrary_float! again, and ran the tests. Only the arm-unknown-linux-gnueabi target failed. So I grab the arguments that made the test fail and put them into a regular test. On the next build, the test passes because we're actually getting the right answer in this case. It's gcc_s and compiler-rt that have the wrong answer.

Either:

  1. There's some subtle bug in the test infrastructure
  2. gcc_s and libcompiler-rt both have the same bug but we don't despite the code being almost identical to compiler-rt
  3. QEMU is broken but only in this specific way
  4. The C compiler's optimizations cause rounding in very small numbers, but rustc does not
  5. Something else?
@alexcrichton
Copy link
Member

Oh that's actually a really good point that we should always compile compiler-rt with optimizations. Right now compiler-rt is conditionally optimized, but gcc_s is pulled in from the system so I think that's always optimize.

@japaric
Copy link
Member

japaric commented Oct 4, 2016

The C compiler's optimizations cause rounding in very small numbers, but rustc does not

Do the errors go away if you test with optimizations enabled (cargo test --release)?

Are these roundings, -ffast-math optimizations? Who's doing the rounding (less precision) C or Rust?

we should always compile compiler-rt with optimizations.

This should be a simple change in the build.rs via gcc::Config (I expect)

But ... enabling optimizations shouldn't change the results of the compiler-rt intrinsics, right? That doesn't sound quite right. You should get the same output regardless of the optimization level.

@mattico
Copy link
Contributor Author

mattico commented Oct 5, 2016

Do the errors go away if you test with optimizations enabled (cargo test --release)?
Are these roundings, -ffast-math optimizations? Who's doing the rounding (less precision) C or Rust?

No, release builds don't change our results. Our answer agrees with wolfram alpha which uses arbitrary precision arithmetic so it seems like compiler-rt and gcc_s have the wrong answer in this case.

I feel like I was a bit unclear about this so here is a table:

Answers for 1.1540496e-38 + -3.59621e-39 on arm-unknown-linux-gnueabi

Method Answer
Wolfram Alpha 7.944286e-39
Rust Constant Folding 7.944286e-39
rustc-builtins 7.944286e-39
gcc_s 4.133629e-39
compiler-rt 4.133629e-39

The check! test gives answers for rustc-builtins, gcc_s, and compiler-rt and the #[test] test is probably using constant folding.

I think it's still possible that gcc is less strict about reordering or combining float operations than rust/llvm, but this page makes it look like that only happens with non-standard flags so perhaps that is not the case.

I'm going to start looking at the disassembly since it doesn't seem like there's a simple answer.

@mattico mattico changed the title Floating point add gives different results when called inside check! macro Floating point add gives different results than compiler-rt on arm-unknown-linux-gnueabi Oct 5, 2016
@mattico
Copy link
Contributor Author

mattico commented Oct 5, 2016

Hmm. The Rust disassembly is much longer and uses many more registers than the C version. I was expecting them to be a bit more similar. I guess it's time to install IDA.

@mattico
Copy link
Contributor Author

mattico commented Oct 5, 2016

Compiling compiler-rt with clang gives an assembly output that is much more similar to the rust version. I'm going to see if it gives the same results.

@japaric
Copy link
Member

japaric commented Oct 5, 2016

Compiling compiler-rt with clang gives an assembly output that is much more similar to the rust version.

That makes given that both clang and rust are based on LLVM.

This sounds like gcc has different defaults around rounding compared to LLVM.

this page makes it look like that only happens with non-standard flags so perhaps that is not the case.

Perhaps try compiling compiler-rt using the "full IEEE 754 compliance" flags: -frounding-math -fsignaling-nans. It seems that gcc defaults to not being fully compliant with IEEE 754. I wonder if LLVM is fully compliant with IEEE 754 by default; that could be the difference.

@mattico
Copy link
Contributor Author

mattico commented Oct 5, 2016

Adding the flags didn't change anything.

@mattico
Copy link
Contributor Author

mattico commented Oct 5, 2016

I used a little C program to test the compiler-rt version of __addsf3 when compiled with different compilers, but all of them give 7.944286e-39.

@japaric
Copy link
Member

japaric commented Oct 5, 2016

Adding the flags didn't change anything.
I used a little C program to test the compiler-rt version of __addsf3 when compiled with different compilers, but all of them give 7.944286e-39.

😕

So, uhmm, calling the same function from C vs from Rust produces different output? Could this be a calling convention problem? Seems unlikely though as you only get different outputs (between C and Rust) for a few inputs. The only other thing I can think about is side effects: it's possible to change the rounding mode using fesetround; perhaps, some pre-main/initialization code is setting that in different ways for C vs for Rust.

@mattico
Copy link
Contributor Author

mattico commented Oct 7, 2016

@japaric Yeah I agree it's a bit strange... I'm still investigating.

Here's a rough summary of what I did (from memory):

$ arm-linux-gnueabi-gcc -O2 -fno-builtins -c addsf3.c # from compiler-rt
$ cat test_arm.c
#include <stdio.h>
extern float __addsf3(float, float);
int main() {
    printf("%e\n", __addsf3(1.1540496e-38, -3.59621e-39));
    return 0;
}
$ arm-linux-gnueabi-gcc -fno-builtins test_arm.c addsf3.o -o test_arm_gcc
$ ./test_arm_gcc # runs in qemu...

And I did the equivalent for clang as well.

I'll look into the crt initialization stuff to see if they mess with floating point rounding.

I guess I should also make an equivalent rust program, which should tell us if the error only happens inside our check! tests for whatever reason.

@mattico
Copy link
Contributor Author

mattico commented Nov 11, 2016

@mattico
Copy link
Contributor Author

mattico commented Nov 12, 2016

I'm currently looking into two possible theories:

  1. japaric suggested a while ago that this is caused by undefined behaviour in compiler-rt (and gcc_s presumably) in floatsisf. I ran the test with the updated compiler-rt which is supposed to fix this issue and nothing changed, but I may have forgotten something. I'm in the process of porting floatsisf anyway.
  2. powi only: don't override arm calling convention compiler-rt#26 (comment) above has to do with arm calling conventions so that possibly applies here. There may be some other calling convention issue as well.

@parched
Copy link
Contributor

parched commented Nov 12, 2016

@mattico I don't think 2. will be the issue as that only affects hard float.

@ithinuel
Copy link
Contributor

I don't know if it may be related to this but i noticed that when the test starts running, QEMU complains about qemu: Unsupported syscall: 384.

ithinuel added a commit to ithinuel/compiler-builtins that referenced this issue Nov 27, 2016
@japaric
Copy link
Member

japaric commented Nov 27, 2016

@ithinuel I see that all the time when running Rust programs compiled for ARM under QEMU. Never bothered investigating but it hasn't been an issue with programs that don't do (too many) floating point operations.

@Amanieu
Copy link
Member

Amanieu commented Nov 27, 2016

That's the getrandom system call, but having it unsupported isn't a big problem since the code just falls back to /dev/urandom.

@japaric
Copy link
Member

japaric commented Nov 29, 2016

So, using a recent nightly, I tested changing the intrinsics that involve floating point arguments/return values from extern "C" to extern "aapcs" but that didn't fix these tests; I still see the same errors on both gnueabi and gnueabihf.

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2016

@japaric Did you change the ABI for the extern block in compiler-builtins/compiler-rt/compiler-rt-cdylib/src/lib.rs? On ARM, all of the intrinsics use the aapcs soft-float calling convention.

@japaric
Copy link
Member

japaric commented Nov 29, 2016

@Amanieu No, I didn't because those functions get immediately turned into usizes so I don't think the CC they are declared with matters.

On ARM, all of the intrinsics use the aapcs soft-float calling convention.

I only tested changing the calling convention of addsf3 and adddf3 as those are the only intrinsics that are causing problems. Here are my changes: https://gist.github.com/japaric/13755ba481a6cd1199fd07f2f32d8c1e

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2016

@japaric This line should use "aapcs" instead of "C":

-            fn $name($f: extern fn($($farg),*) -> $fret,
+            fn $name($f: extern "C" fn($($farg),*) -> $fret,

EDIT: ah nevermind, I see you duplicated the whole thing just for aapcs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants