Skip to content

Conversion from&to float<->integer (32&64 bits) #139

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

Closed
wants to merge 4 commits into from

Conversation

ithinuel
Copy link
Contributor

@ithinuel ithinuel commented Feb 5, 2017

Here is the continuation of PR #121.

The tests that pass are actually where the new functions are not tested.
And the tests that fails are on UB results.
This implementation always saturate to the closest min_value/max_value.

@japaric
Copy link
Member

japaric commented Feb 6, 2017

And the tests that fails are on UB results.

You can restrict the range (return None) to not hit UB with the gcc implementation.

This implementation always saturate to the closest min_value/max_value.

Is that what compiler-rt implementation does?

@ithinuel
Copy link
Contributor Author

ithinuel commented Feb 6, 2017

You can restrict the range (return None) to not hit UB with the gcc implementation.

Thank you ! I excluded values out of integer's range and NaN.

Is that what compiler-rt implementation does?

Yes when SOFT-FP is used.

@ithinuel
Copy link
Contributor Author

ithinuel commented Feb 6, 2017

I don't understand why the emulated hf returned an invalid value :

---- float::conv::tests::_test::__fixsfsi stdout ----
	__fixsfsi - Args: 2147483600 
 compiler-builtins: Some(2147483647)
 compiler_rt:       Some(-2147483648)
 gcc_s:             None

Everything passed on my repos, https://travis-ci.org/ithinuel/compiler-builtins/builds/198734216
Should I limit tests to archs with soft-fp ?

@japaric
Copy link
Member

japaric commented Feb 6, 2017

I don't understand why the emulated hf returned an invalid value

So, my hypothesis is that since armhf doesn't use the e.g. fixsfsi intrinsic then it simply may not be correctly implemented in compiler-rt.

Should I limit tests to archs with soft-fp ?

I think we should both not expose and not test, on armhf, the conversion intrinsics that are not used on armhf. For example: __aeabi_f2lz (f32 -> i64) should be exposed and tested on arm-gnueabi and on armv7-gnueabihf (as it's needed on both platforms) whereas __aeabi_f2iz (f32 -> i32) should be exposed only on arm-gnueabi (as it's not needed on armv7-gnueabihf).

@ithinuel
Copy link
Contributor Author

ithinuel commented Feb 7, 2017

According to the Rust Reference there is no way to distinguish if the target is armhf and/or which kind of fpu is available (and which operation are provided by hardware).

Do you know an annotation that could be used to apply this kind of filter ?

@japaric
Copy link
Member

japaric commented Feb 7, 2017

Do you know an annotation that could be used to apply this kind of filter ?

You can create a "cfg" in the build script. Examples. Then you can use e.g. cfg(thumbv6m) in the code.

and which operation are provided by hardware

You can cross compile bin/intrinsics.rs for the ARM targets and see (objdump) which operation lowers to an intrinsic and which one lowers to instructions.

@bors
Copy link
Contributor

bors commented Feb 8, 2017

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

@FenrirWolf
Copy link

Can other ARM aliases be added to this PR? I came across the need for __aeabi_ul2d and __aeabi_d2ulz in a program I'm working on, which are aliases to __floatundidf and __fixunsdfdi.

@japaric
Copy link
Member

japaric commented Feb 15, 2017

ping @ithinuel, any update on this?

@ithinuel
Copy link
Contributor Author

ithinuel commented Feb 16, 2017

@FenrirWolf Yes, I will add all necessary aliases for aeabi.
@japaric I am looking at what you pointed in your last comment. I saw that

    #[cfg(thumbv6m)]
    pub fn aeabi_f2lz(_: f32) -> i64 {
        0
    }

It seems that compiler-rt is missing some functions to fully support thumbv6m devices :/
We will probably end up by building for thumbv7m, thumbv6m but only test for thumbv7m if it has no hardfloat.
I still need to investigate on that.

@japaric
Copy link
Member

japaric commented Feb 18, 2017

It seems that compiler-rt is missing some functions to fully support thumbv6m devices

Yeah, see #75.

but only test for thumbv7m

We can't properly test the thumb targets anyway as there no way to compile the test crate for them. The only test we have is bin/intrinsics which checks for missing intrinsics.

bors added a commit that referenced this pull request Apr 8, 2017
Conversion from&to float<->integer

this is a rebased version of #139

cc @ithinuel
bors added a commit that referenced this pull request Apr 8, 2017
Conversion from&to float<->integer

this is a rebased version of #139

cc @ithinuel
bors added a commit that referenced this pull request Apr 8, 2017
Conversion from&to float<->integer

this is a rebased version of #139

cc @ithinuel
@japaric
Copy link
Member

japaric commented Apr 8, 2017

Rebased version landed in #147 🎉. Thanks for working on this, @ithinuel!

@japaric japaric closed this Apr 8, 2017
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.

4 participants