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.

Sign up for GitHub

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

Jump to bottom

i128 intrinsics #133

Merged
merged 11 commits into from
Feb 4, 2017
Merged

i128 intrinsics #133

merged 11 commits into from
Feb 4, 2017

Conversation

est31
Copy link
Member

@est31 est31 commented Jan 4, 2017 •
edited
Loading

Adds i128 intrinsics.

Note that this PR doesn't do float intrinsics, due to the missing presence of macros, those are however still required in order for rustc to switch to this crate.

@est31
Copy link
Member Author

est31 commented Jan 4, 2017

Builds fail because they are still on an outdated nightly (in fact, nightly with i128 support is released yet).

I'm currently only developing with RUSTFLAGS="--cfg stage0" cargo test --no-default-features, and after nightlies will be available, I'll drop the RUSTFLAGS argument.

@est31 est31 force-pushed the i128 branch 9 times, most recently from ae515d0 to d053642 Compare January 5, 2017 11:00
@est31 est31 mentioned this pull request Jan 5, 2017
Closed
8 tasks
@est31
Copy link
Member Author

est31 commented Jan 5, 2017

Now still to do: a) unit tests b) better stage0 support. Then I'm ready for review :)

@est31 est31 force-pushed the i128 branch 5 times, most recently from c6cc332 to d3b0497 Compare January 6, 2017 06:03
@est31
Copy link
Member Author

est31 commented Jan 6, 2017

Okay, I think adding tests is a little bit complicated, as the tests will only work on 64 bit non windows platforms... Is it okay if this doesn't get quickcheck like tests for now? if yes, r? @japaric

@est31 est31 changed the title [wip] i128 intrinsics i128 intrinsics Jan 6, 2017
@est31 est31 mentioned this pull request Jan 17, 2017
Closed
@est31 est31 force-pushed the i128 branch 2 times, most recently from e9d01fb to b78e956 Compare February 3, 2017 22:46
@est31
Copy link
Member Author

est31 commented Feb 3, 2017

@japaric I've added quickcheck tests. r?

japaric
japaric reviewed Feb 4, 2017
View reviewed changes
src/int/mul.rs Outdated
let mut overflow = 2;
let r = f(a, b, &mut overflow);
if overflow != 0 && overflow != 1 {
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't a value different that 0 or 1 indicate a bug in our implementation? I think such case should panic instead of being skipped.

japaric
japaric reviewed Feb 4, 2017
View reviewed changes
src/int/udiv.rs
if d == 0 {
None
} else {
// FIXME fix the segfault when the remainder is requested
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you open an issue about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@japaric
Copy link
Member

japaric commented Feb 4, 2017

Thanks, @est31. r=me with with the overflow change.

@est31
Copy link
Member Author

est31 commented Feb 4, 2017

Changed it. Note that travis failed due to bug #136.

@est31 est31 mentioned this pull request Feb 4, 2017
Closed
@japaric
Copy link
Member

japaric commented Feb 4, 2017

Changed it. Note that travis failed due to bug #136.

That should be fixed in #138 so I'm going to put this in the queue.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 4, 2017

📌 Commit f8a4e3f has been approved by japaric

@bors
Copy link
Contributor

bors commented Feb 4, 2017

⌛ Testing commit f8a4e3f with merge ca6bb63...

bors added a commit that referenced this pull request Feb 4, 2017
@bors
Auto merge of #133 - est31:i128, r=japaric
ca6bb63 
i128 intrinsics

Adds i128 intrinsics.

Note that this PR doesn't do float intrinsics, due to the missing presence of macros, those are however still required in order for rustc to switch to this crate.
@bors
Copy link
Contributor

bors commented Feb 4, 2017

💔 Test failed - status-travis

@japaric
Copy link
Member

japaric commented Feb 4, 2017

I have manually cleared the caches. Let's try again.

@bors retry

@bors
Copy link
Contributor

bors commented Feb 4, 2017

⌛ Testing commit f8a4e3f with merge 547b938...

bors added a commit that referenced this pull request Feb 4, 2017
@bors
Auto merge of #133 - est31:i128, r=japaric
547b938 
i128 intrinsics

Adds i128 intrinsics.

Note that this PR doesn't do float intrinsics, due to the missing presence of macros, those are however still required in order for rustc to switch to this crate.
@est31
Copy link
Member Author

est31 commented Feb 4, 2017

hmm seems that mips64-unknown-linux-gnuabi64 and mips64el-unknown-linux-gnuabi64 tests failed for __modti3 and __divti3:

__divti3 - Args: 119123594998676976834373841781095175696 75802332446715572479975239465689939968 
 compiler-builtins: Some(-102509915410633448775648222814998102015)
 compiler_rt:       Some(1)
 gcc_s:             Some(Some(1))
__divti3 - Args: 59561797499338488417186920890547587848 75802332446715572479975239465689939968 

 compiler-builtins: Some(-51254957705316724387824111407499051008)
 compiler_rt:       Some(0)
 gcc_s:             Some(Some(0))

and

__modti3 - Args: 27708956818918212506987268272516826162 322710947762631927081672291152409133056 
 compiler-builtins: Some(10137537660611676125284951993157747760)
 compiler_rt:       Some(10137537660611676125284951993157747762)
 gcc_s:             Some(Some(10137537660611676125284951993157747762))
__modti3 - Args: 13854478409459106253493634136258413081 322710947762631927081672291152409133056 
 compiler-builtins: Some(13854478409459106253493634136258413080)
 compiler_rt:       Some(13854478409459106253493634136258413081)
 gcc_s:             Some(Some(13854478409459106253493634136258413081))

@bors
Copy link
Contributor

bors commented Feb 4, 2017

💔 Test failed - status-travis

@est31
Copy link
Member Author

est31 commented Feb 4, 2017

Well seems mips does not support i128 integers on clang, which means we should disable it anyway. Will disable all quickcheck tests for mips.

@est31
Copy link
Member Author

est31 commented Feb 4, 2017

r? @japaric

@est31
Disable quickcheck tests on mips
37d3490 
Two reasons:
    * the C versions __divti3 and __modti3 are apparently broken,
      at least when used in quickcheck. They change their own arguments.
    * compiler_rt's support for mips is disabled already on clang [1].
      Its desireable to support working "cargo test" on that compiler
      as well, and not greet the tester with linker errors.

[1]: http://llvm.org/viewvc/llvm-project?view=revision&revision=224488
@japaric
Copy link
Member

japaric commented Feb 4, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Feb 4, 2017

📌 Commit 37d3490 has been approved by japaric

@bors
Copy link
Contributor

bors commented Feb 4, 2017

⌛ Testing commit 37d3490 with merge 9debde0...

bors added a commit that referenced this pull request Feb 4, 2017
@bors
Auto merge of #133 - est31:i128, r=japaric
9debde0 
i128 intrinsics

Adds i128 intrinsics.

Note that this PR doesn't do float intrinsics, due to the missing presence of macros, those are however still required in order for rustc to switch to this crate.
@bors
Copy link
Contributor

bors commented Feb 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 9debde0 to master...

@bors bors merged commit 37d3490 into rust-lang:master Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@japaric japaric japaric left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@est31 @japaric @bors