Implement comparesf2/comparedf2 intrinsics #222

whitequark · 2017-12-28T03:43:18Z

These intrinsics are indeed called on soft-float targets, you can verify this by compiling:

#![no_std]
fn comparesf2(a: f32, b: f32) -> bool { a > b }
fn comparedf2(a: f64, b: f64) -> bool { a > b }

and looking at the disassembly.

Doesn't come with tests because I'm not about to write fourteen of these horrifying build.rs structs by hand, and also because cargo test fails on latest nightly.

E.g. take a look at the assembly output for: pub fn comparesf2(a: f32, b: f32) -> bool { a > b } pub fn comparedf2(a: f64, b: f64) -> bool { a > b } which will include calls to __gtsf2 and __gtdf2.

alexcrichton · 2017-12-28T16:26:13Z

Thanks! Could at least one test be written? Exercising at least a few of the code paths? Additionally can you gist the error that cargo test has on nightly?

Finally, should these return bool at the ABI level? Could i32 be used for explicitness perhaps?

alexcrichton · 2017-12-28T16:26:28Z

Oh and also, do we compile the C version of these intrinsics? If so, can those files be removed from compilation?

whitequark · 2017-12-29T06:12:49Z

The CI error is an ICE:

error: internal compiler error: /checkout/src/librustc_metadata/cstore_impl.rs:131: get_optimized_mir: missing MIR for `DefId(3/0:10 ~ utest_cortex_m_qemu[aedb]::lang_items[0]::start[0])

The error on nightly (rust version 1.24.0-nightly (77e189cd7 2017-12-28)):

error[E0152]: duplicate lang item found: `i128_add`.
   --> src/macros.rs:292:9
    |
292 | /         pub fn $name( $($argname:  $ty),* ) -> $ret {
293 | |             $($body)*
294 | |         }
    | |_________^
    | 
   ::: src/int/addsub.rs
    |
82  | / u128_lang_items! {
83  | |     #[lang = "i128_add"]
84  | |     pub fn rust_i128_add(a: i128, b: i128) -> i128 {
85  | |         rust_u128_add(a as _, b as _) as _
...   |
123 | |     }
124 | | }
    | |_- in this macro invocation
    |
    = note: first defined in crate `compiler_builtins`.

[lots more of similar errors]

Note that this changes semantics: pub extern "C" fn __eqsf2(a: f32, b: f32) -> bool { cmp(a, b).to_le_abi() != 0 } is not the same as pub extern "C" fn __eqsf2(a: f32, b: f32) -> i32 { cmp(a, b).to_le_abi() } However, compiler-rt does the latter, so this is actually an improvement.

whitequark · 2017-12-29T06:18:32Z

Finally, should these return bool at the ABI level? Could i32 be used for explicitness perhaps?

Good catch, this is actually a divergence from compiler-rt's ABI, since the values are different too (consider the -1 case). I don't think it actually matters but it is a good thing to stay consistent.

Oh and also, do we compile the C version of these intrinsics? If so, can those files be removed from compilation?

Done.

whitequark · 2017-12-29T06:27:08Z

There's also another test failure I get when running locally, if I comment out the cfg_attr in the u128_lang_items macro:

error: couldn't read "/home/whitequark/Projects/compiler-builtins/target/debug/build/compiler_builtins-ef464c3bcc7b9509/out/udivdi3.rs": No such file or directory (os error 2)
 --> tests/udivdi3.rs:8:1
  |
8 | include!(concat!(env!("OUT_DIR"), "/udivdi3.rs"));
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Could not compile `compiler_builtins`.
warning: build failed, waiting for other jobs to finish...
error: couldn't read "/home/whitequark/Projects/compiler-builtins/target/debug/build/compiler_builtins-ef464c3bcc7b9509/out/fixunssfti.rs": No such file or directory (os error 2)
 --> tests/fixunssfti.rs:8:1
  |
8 | include!(concat!(env!("OUT_DIR"), "/fixunssfti.rs"));
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

whitequark · 2017-12-29T06:36:48Z

Ok it looks like the quickcheck style tests only run on cargo test --features gen-tests? And then they succeed on my nightly. I've updated README to mention that.

whitequark · 2017-12-29T07:50:46Z

Thanks! Could at least one test be written? Exercising at least a few of the code paths?

Excellent catch! There actually was a bug related to signedness handling. I've added tests (not for unord?f2 since that would just be ignored anyway, and not for aliases since that's kinda pointless).

alexcrichton · 2017-12-29T16:20:29Z

Thanks! Looks like the failure on arm is relevant?

whitequark · 2017-12-29T18:07:52Z

Fixed

alexcrichton · 2017-12-30T17:21:01Z

build.rs

@@ -5127,8 +5443,6 @@ mod c {
                    "arm/bswapsi2.S",
                    "arm/clzdi2.S",
                    "arm/clzsi2.S",
-                    "arm/comparesf2.S",
-                    "arm/divmodsi4.S",


Was this deletion accidental perhaps? (or is the assembly weird like that?)

alexcrichton · 2018-01-12T22:10:30Z

@bors: r+

bors · 2018-01-12T22:10:31Z

📌 Commit 205322b has been approved by alexcrichton

bors · 2018-01-12T22:10:37Z

⌛ Testing commit 205322b with merge 1d895cd...

Implement comparesf2/comparedf2 intrinsics These intrinsics are indeed called on soft-float targets, you can verify this by compiling: ```rust #![no_std] fn comparesf2(a: f32, b: f32) -> bool { a > b } fn comparedf2(a: f64, b: f64) -> bool { a > b } ``` and looking at the disassembly. Doesn't come with tests because I'm not about to write *fourteen* of these horrifying build.rs structs by hand, and also because `cargo test` fails on latest nightly.

bors · 2018-01-13T09:07:11Z

💥 Test timed out

alexcrichton · 2018-01-13T17:52:14Z

@bors: retry

bors · 2018-01-13T17:52:21Z

⌛ Testing commit 205322b with merge 544e517...

Implement comparesf2/comparedf2 intrinsics These intrinsics are indeed called on soft-float targets, you can verify this by compiling: ```rust #![no_std] fn comparesf2(a: f32, b: f32) -> bool { a > b } fn comparedf2(a: f64, b: f64) -> bool { a > b } ``` and looking at the disassembly. Doesn't come with tests because I'm not about to write *fourteen* of these horrifying build.rs structs by hand, and also because `cargo test` fails on latest nightly.

bors · 2018-01-13T19:58:55Z

💔 Test failed - status-travis

alexcrichton · 2018-01-13T23:27:53Z

Ah right haven't fixed that yet...

alexcrichton · 2018-01-25T00:17:47Z

@whitequark mind helping diagnosing a few issues I think are with this PR?

I believe this PR is the cause of this failure, notably on ARM Android we're getting a number of weird floating point results. I tried to correct what I thought were some typos, namely defining our own aeabi aliases (not using compiler-rt's) and fixing the arugments on __gtdf2, but that ended up causing even more regressions locally.

Do you know what's going on here? Do you know if there's an ABI mismatch perhaps with how floats are being passed? Have you tested this locally for the target you're working with?

whitequark · 2018-01-25T00:44:19Z

Have you tested this locally for the target you're working with?

Yes, for OR1K, it's out-of-tree.

Do you know if there's an ABI mismatch perhaps with how floats are being passed?

Maybe the arguments are swapped? Just like for __aeabi_memcpy and friends.

alexcrichton · 2018-01-25T01:02:36Z

Hm ok, it looks like the __aeabi ABI is slightly different not with arguments but at least with return value, I'm testing that out to see if it resolves the problems I found.

I still think though that __gtdf2 needs to be updated, right?

whitequark · 2018-01-25T01:09:16Z

Yes, that's obviously correct.

whitequark added 2 commits December 28, 2017 02:57

comparesf2/comparedf2 are called on soft-float targets.

82ee1dd

E.g. take a look at the assembly output for: pub fn comparesf2(a: f32, b: f32) -> bool { a > b } pub fn comparedf2(a: f64, b: f64) -> bool { a > b } which will include calls to __gtsf2 and __gtdf2.

Implement comparesf2/comparedf2 intrinsics.

f5d8647

whitequark added 2 commits December 29, 2017 06:14

comparesf2/comparedf2: do not build the C versions of intrinsics.

3d1600b

README: update to explain how to run quickcheck-style tests.

4cf6571

comparesf2/comparedf2: fix a signedness bug and add tests.

07b446a

alexcrichton reviewed Dec 30, 2017

View reviewed changes

comparesf2: do not build the assembly versions of intrinsics on ARM.

205322b

alexcrichton merged commit bf912e6 into rust-lang:master Jan 13, 2018

This was referenced Jan 24, 2018

Tracking issue for LLVM 6 upgrade rust-lang/rust#47683

Closed

Update compiler-builtins submodule rust-lang/rust#47711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement comparesf2/comparedf2 intrinsics #222

Implement comparesf2/comparedf2 intrinsics #222

whitequark commented Dec 28, 2017

alexcrichton commented Dec 28, 2017

alexcrichton commented Dec 28, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

alexcrichton commented Dec 29, 2017

whitequark commented Dec 29, 2017

alexcrichton Dec 30, 2017

whitequark Dec 31, 2017

whitequark Jan 12, 2018

alexcrichton commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 13, 2018

alexcrichton commented Jan 25, 2018

whitequark commented Jan 25, 2018

alexcrichton commented Jan 25, 2018

whitequark commented Jan 25, 2018

Implement comparesf2/comparedf2 intrinsics #222

Implement comparesf2/comparedf2 intrinsics #222

Conversation

whitequark commented Dec 28, 2017

alexcrichton commented Dec 28, 2017

alexcrichton commented Dec 28, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

whitequark commented Dec 29, 2017

alexcrichton commented Dec 29, 2017

whitequark commented Dec 29, 2017

alexcrichton Dec 30, 2017

Choose a reason for hiding this comment

whitequark Dec 31, 2017

Choose a reason for hiding this comment

whitequark Jan 12, 2018

Choose a reason for hiding this comment

alexcrichton commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 13, 2018

alexcrichton commented Jan 25, 2018

whitequark commented Jan 25, 2018

alexcrichton commented Jan 25, 2018

whitequark commented Jan 25, 2018