Add float quickcheck #111

mattico · 2016-10-16T21:26:13Z

No description provided.

japaric

I had already review this but there are some changes that are needed to not include the intrinsics that are failing their tests (for the arm-unknown-linux-gnueabi target only).

japaric · 2016-10-16T21:50:47Z

src/float/add.rs

+                    a: F32,
+                    b: F32)
+                    -> Option<F32> {
+            if option_env!("TARGET") != Some("arm-unknown-linux-gnueabi") {


This doesn't seem to be working; Travis is still failing this test.

Could you instead #[cfg] away the addsf3 and adddf3 symbols along with these too tests for the arm-unknown-linux-gnueabi? And please add a comment about the problem and point to the issue that's tracking it #90

japaric · 2016-10-16T21:52:32Z

src/qc.rs

+                // just avoid testing against gcc_s on those targets. Do note that our
+                // implementation matches the output of the FPU instruction on *hard* float targets
+                // and matches its gcc_s counterpart on *soft* float targets.
+                if cfg!(gnueabihf) {


Is this still necessary? There's now a FRepr struct wrapper for this logic.

This was meant to replace FRepr, but I forgot to do that when rebasing.

mattico · 2016-10-16T22:46:52Z

I hope you don't have build failure emails turned on 😉. I think I fixed everything now...

japaric · 2016-10-16T23:13:43Z

I hope you don't have build failure emails turned on 😉.

Oh, they are enabled but they aren't reaching my mailbox which probably means they are being sent to the rust-lang org mailbox 😄.

japaric · 2016-10-16T23:17:02Z

src/float/add.rs

@@ -181,117 +181,30 @@ macro_rules! add {
    }
 }

+// NOTE(cfg) These are disabled for gnueabihf due to 
+// https://github.com/rust-lang-nursery/compiler-builtins/issues/90
+#[cfg(not(gnueabi))]


Right now, cfg(gnueabihf) works but cfg(gnueabi) doesn't. You'll have to update the build.rs to make Cargo pass this cfg when the target is arm-unknown-linux-gnueabi. See https://github.com/rust-lang-nursery/compiler-builtins/blob/master/build.rs#L428

So should the addsf3.c files be enabled for arm-unknown-linux-gnueabi since we're disabling the rust version?

mattico · 2016-10-27T21:26:43Z

Oops, I've been neglecting this. Will update.

japaric · 2016-11-08T04:55:12Z

@bors r+

Thank you, @mattico

bors · 2016-11-08T04:55:14Z

📌 Commit 6504950 has been approved by japaric

bors · 2016-11-08T04:55:17Z

⌛ Testing commit 6504950 with merge e871d09...

Add float quickcheck

bors · 2016-11-08T15:01:20Z

💥 Test timed out

japaric · 2016-11-08T15:45:36Z

@alexcrichton The tests seemed to pass (they are "green") but bors/homu didn't merge it.

alexcrichton · 2016-11-08T16:02:00Z

@bors: retry force clean

Your guess is as good as mine...

alexcrichton · 2016-11-11T16:25:57Z

@bors: retry clean

bors · 2016-11-11T16:25:59Z

⌛ Testing commit 6504950 with merge 93babe2...

Add float quickcheck

bors · 2016-11-12T03:01:26Z

💥 Test timed out

japaric · 2016-11-12T03:56:46Z

@alexcrichton Time out again. Maybe something got misconfigured (the webhook?).

alexcrichton · 2016-11-12T05:31:40Z

Hm so recently I reclassified the appveyor repo to run under a different account, and I've been seeing these same timeout problems on the libc repo as well.

Unfortunately I have no idea why they're happening... Appveyor/Travis are green though, so want to merge?

japaric · 2016-11-12T05:41:56Z

Hm so recently I reclassified the appveyor repo to run under a different account, and I've been seeing these same timeout problems on the libc repo as well.

Perhaps you should try removing the existing appveyor webhooks (this repo > settings > webhooks) and re-adding the projects on appveyor.

Appveyor/Travis are green though, so want to merge?

👍 from me.

alexcrichton · 2016-11-12T18:48:23Z

Let's try that one more time...

@bors: retry

Add float quickcheck

bors · 2016-11-12T18:48:25Z

⌛ Testing commit 6504950 with merge 14ed7ef...

mattico · 2016-11-12T20:25:01Z

Hm, looking at this now I think I should unify the handling of *-gnueabi and *-gnueabihf since (AFAIK) the error is the same in both cases. That is, for both targets:

Continue to use the rust add implementation since it gives correct answers
Disable tests for the add impl on *-gnueabi* since compiler-rt and gcc_s give incorrect answers sometimes (I still haven't figured out why).

If this PR gets merged before you see this, I'll just make a follow-up PR.

mattico · 2016-11-12T20:37:09Z

This probably won't work but:
@bors r-

japaric · 2016-11-12T20:41:58Z

@bors r-

japaric · 2016-11-12T20:42:37Z

I think bors is not listening to me either...

japaric · 2016-11-12T20:43:14Z

but it seems bors got stuck again anyway so this won't get merged.

mattico · 2016-11-12T21:01:29Z

Ok this is much better now. It's simpler, and now the powi tests will still run on gnueabi.

japaric · 2016-11-12T21:17:01Z

build.rs

-    if llvm_target.last() == Some(&"gnueabihf") {
-        println!("cargo:rustc-cfg=gnueabihf")
+    if llvm_target.last().unwrap().contains("gnueabi") {
+        println!("cargo:rustc-cfg=gnueabi")


Could you use two cfgs here, gnueabi and gnueabihf, instead of overloading the word "gnueabi" to mean both ABIs? I think it would be less error prone that way.

Hm. Good point. This also accidentally disables the tests on armv7-* which doesn't seem to have this issue (does it?).

Nevermind, armv7-* also has this issue.

mattico · 2016-11-12T21:31:57Z

build.rs

-        println!("cargo:rustc-cfg=gnueabi")
+    if llvm_target[0].starts_with("arm") &&
+       llvm_target.last().unwrap().contains("gnueabi") {
+        println!("cargo:rustc-cfg=arm-linux")


@japaric is this better?

japaric · 2016-11-12T22:12:36Z

@bors r+

Thanks, @mattico.

bors · 2016-11-12T22:12:37Z

📌 Commit f68475e has been approved by japaric

bors · 2016-11-12T22:12:38Z

⌛ Testing commit f68475e with merge c459e19...

Add float quickcheck

mattico · 2016-11-12T22:14:07Z

🍀

japaric · 2016-11-13T02:17:14Z

Stuck again but I'm going to merge manually.

111: Implement tanh r=japaric a=porglezomp Closes rust-lang#37 Co-authored-by: C Jones <code@calebjones.net>

japaric approved these changes Oct 16, 2016

View reviewed changes

mattico force-pushed the new_float_quickcheck branch 2 times, most recently from fee97fa to 585d6ed Compare October 16, 2016 22:46

mattico force-pushed the new_float_quickcheck branch from 585d6ed to 3e7fb1f Compare October 16, 2016 23:13

japaric reviewed Oct 16, 2016

View reviewed changes

mattico force-pushed the new_float_quickcheck branch from 3e01b2e to 6d6dbdc Compare November 3, 2016 02:32

bors added a commit that referenced this pull request Nov 8, 2016

Auto merge of #111 - mattico:new_float_quickcheck, r=japaric

e871d09

Add float quickcheck

bors added a commit that referenced this pull request Nov 11, 2016

Auto merge of #111 - mattico:new_float_quickcheck, r=japaric

93babe2

Add float quickcheck

bors added a commit that referenced this pull request Nov 12, 2016

Auto merge of #111 - mattico:new_float_quickcheck, r=japaric

14ed7ef

Add float quickcheck

Add float quickcheck

655f642

mattico force-pushed the new_float_quickcheck branch from 6504950 to 655f642 Compare November 12, 2016 21:00

japaric reviewed Nov 12, 2016

View reviewed changes

mattico mentioned this pull request Nov 12, 2016

Implement floatunsisf (WIP) #106

Closed

Use correct lib name in qc test failure

77ca63c

mattico commented Nov 12, 2016

View reviewed changes

Use better cfg name for arm-linux tests

f68475e

mattico force-pushed the new_float_quickcheck branch from 691782d to f68475e Compare November 12, 2016 21:53

bors added a commit that referenced this pull request Nov 12, 2016

Auto merge of #111 - mattico:new_float_quickcheck, r=japaric

c459e19

Add float quickcheck

japaric merged commit 8992316 into rust-lang:master Nov 13, 2016

mattico deleted the new_float_quickcheck branch November 13, 2016 02:23

tgross35 pushed a commit to tgross35/compiler-builtins that referenced this pull request Feb 23, 2025

Merge rust-lang#111

8da2707

111: Implement tanh r=japaric a=porglezomp Closes rust-lang#37 Co-authored-by: C Jones <code@calebjones.net>

Add float quickcheck #111

Add float quickcheck #111

Conversation

mattico commented Oct 16, 2016

japaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattico commented Oct 16, 2016 • edited Loading

japaric commented Oct 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattico commented Oct 27, 2016

japaric commented Nov 8, 2016

bors commented Nov 8, 2016

bors commented Nov 8, 2016

bors commented Nov 8, 2016

japaric commented Nov 8, 2016

alexcrichton commented Nov 8, 2016

alexcrichton commented Nov 11, 2016

bors commented Nov 11, 2016

bors commented Nov 12, 2016

japaric commented Nov 12, 2016

alexcrichton commented Nov 12, 2016

japaric commented Nov 12, 2016

alexcrichton commented Nov 12, 2016

bors commented Nov 12, 2016

mattico commented Nov 12, 2016

mattico commented Nov 12, 2016

japaric commented Nov 12, 2016

japaric commented Nov 12, 2016

japaric commented Nov 12, 2016

mattico commented Nov 12, 2016

Choose a reason for hiding this comment

mattico Nov 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japaric commented Nov 12, 2016

bors commented Nov 12, 2016

bors commented Nov 12, 2016

mattico commented Nov 12, 2016

japaric commented Nov 13, 2016

mattico commented Oct 16, 2016 •

edited

Loading

mattico Nov 12, 2016 •

edited

Loading