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

Implement floatunsisf (WIP) #106

Closed
wants to merge 3 commits into from
Closed

Conversation

Phaiax
Copy link
Contributor

@Phaiax Phaiax commented Oct 15, 2016

PR for review and travis.
I will delete the comments if you say that the approach is correct.

I'll do fmul and f2uiz too, because my project is complaining^^

@Phaiax
Copy link
Contributor Author

Phaiax commented Oct 15, 2016

Another question:
compiler-rt defines an alias for many functions: ARM_EABI_FNALIAS(ui2f, floatunsisf)
See #define
Because my project complained about the alias (undefined reference to__aeabi_ui2f'`) I guess the aliases are needed.

#[inline(always)]
#[cfg_attr(not(test), no_mangle)]
pub extern "C" fn $alias(a: $ity) -> $fty {
    $intrinsic(a)
}

Like this? But I guess this will not work since LLVM expects a symbol. asm!?

@Phaiax
Copy link
Contributor Author

Phaiax commented Oct 15, 2016

I managed to test locally and now I understand the testing system better :D

For the target arm-unknown-linux-gnueabihf I get confusing results. The compiler-builtins answer is correct but the other two seem confusing.

Test resuls
libcompiler-rt disassembly
libcompiler_rt_cdylib.so dissassembly

Currently I guess there is something wrong with the calling convention. The result is arbitrary (but constant on repeated runs with the same source code)

    #[test]
    fn test_floatunsisf_libcompilerrt() {
        let compiler_rt_fn = ::compiler_rt::get("__floatunsisf");
        let compiler_rt_fn : extern fn(u32) -> f32 = unsafe { mem::transmute(compiler_rt_fn) };
        println!("1231515 {:?}", compiler_rt_fn);
        let ans = compiler_rt_fn(0);
        println!("{:?}", ans);
        assert!(ans.eq_repr(0.0f32));

    }

gives

---- float::conv::tests::test_floatunsisf_libcompilerrt stdout ----
    1231515 0xf650a5e9
0.0000000006592472
thread 'float::conv::tests::test_floatunsisf_libcompilerrt' panicked at 'assertion failed: ans.eq_repr(0.0f32)', src/float/conv.rs:90

And the same code without the println!("123... gives

---- float::conv::tests::test_floatunsisf_libcompilerrt stdout ----
    17867484000000000000000000
thread 'float::conv::tests::test_floatunsisf_libcompilerrt' panicked at 'assertion failed: ans.eq_repr(0.0f32)', src/float/conv.rs:91

::compiler_rt::get("__aeabi_ui2f"); does not work because there is no symbol __aeabi_ui2f in compiler_rt. (see second dissassembly). The results differ between release and debug mode but are still arbitrary.

I guess either the calling convention is wrong or the linker does something bad (it removes the __aeabi_ui2f symbol at least).

@mattico
Copy link
Contributor

mattico commented Oct 16, 2016

@Phaiax those test failures seem spookily similar to #90

A calling convention problem seems plausible to me. I'll look into that.

@mattico
Copy link
Contributor

mattico commented Oct 16, 2016

Also we haven't found a perfect way to do function aliases yet (if one even exists), so having a wrapper function is what we do for now. Those should probably be marked as inline always like you've done.

@japaric
Copy link
Member

japaric commented Oct 16, 2016

For the target arm-unknown-linux-gnueabihf I get confusing results.

Ugh, this target keeps causing problems. Perhaps, we could conditionally not compile some intrinsics for this target until we figure out what the problem is. That way we can continue landing changes without being blocked by this issue. @alexcrichton what do you think?

Reminder for @mattico: please re-send PR #48 when you get the chance 😄

@mattico
Copy link
Contributor

mattico commented Oct 16, 2016

So on arm there are 3 different floating point ABIs:

  • soft: Software floating point, passes results in integer registers (since there are no fp registers).
  • softfp: Software or hardware floating point, but passes results in integer registers.
  • hard: Hardware floating point, passes results in VFP registers.

It appears that libgcc is built for softfp if I understand it's build system (I don't). It seems possible that Rust is using hard and some random intermediate value left in a VFP register is used as the argument.

@alexcrichton
Copy link
Member

@japaric seems reasonable to me yeah!

@mattico
Copy link
Contributor

mattico commented Nov 12, 2016

I think all this PR needs is to be updated to use quickcheck and to disable tests on the failing targets. See:
https://github.com/mattico/compiler-builtins/blob/655f642d3f90d59ea008da9254566d4344ed8cba/src/float/add.rs#L192

If you're short on time I can update this PR once #111 lands.

@bors
Copy link
Contributor

bors commented Nov 13, 2016

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

@japaric
Copy link
Member

japaric commented Apr 8, 2017

A more complete version of this PR landed in #147. Sorry that it took so long to get this done :-/.

@japaric japaric closed this Apr 8, 2017
tgross35 pushed a commit to tgross35/compiler-builtins that referenced this pull request Feb 23, 2025
106: implement fmaf r=japaric a=erikdesjardins

closes rust-lang#20

Co-authored-by: Erik <[email protected]>
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 this pull request may close these issues.

5 participants