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

move to the WIP Rust port of compiler-rt #37802

Closed
wants to merge 3 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 16, 2016

Couldn't reopen #36992 for some reason so I'm sending a new PR.

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@TimNN
Copy link
Contributor

TimNN commented Nov 16, 2016

Just to make sure, does this handle the floating point intrinsics on armhf correctly? (cc #37559, #37630)

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

@TimNN Right now, compiler-builtins uses extern "C" fn for all the instrinsics but it doesn't implement floaundif (in Rust). I understand that floatundif must be marked as extern "aapcs" fn for eabihf targets; is there any other intrinsic that will need that?

Also, I think that extern "C" fn is equivalent to extern "aapcs" fn on eabi targets. So, we only need to be extra careful with the eabihf targets (where LLVM mixes the soft (intrinsics) and hard (global setting) calling conventions for some reason)

I think that we have to update the compiler-rt submodule in the compiler-builtins repo though. To pick up the fixes (on the C side) for the bugs you linked.

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

make tidy will fail. It should ignore the C code inside the compiler-builtins repo. Then I have to fix all the real errors in the Rust code of the compiler-builtins repo.

@TimNN
Copy link
Contributor

TimNN commented Nov 16, 2016

I think that extern "C" fn is equivalent to extern "aapcs" fn on eabi targets

I think so as well.

So, from what I remember every thing which has a __aeabi_ alias must use the soft float calling convention on all arm targets, even hard float ones.

Taking a quick look at the rust compiler-rt sources, I think that the only intrinsics that could currently be problematic are the __aeabi_(s|d)add ones, which I would hope are never called for the hard float targets, so we are probably fine.

I think that we have to update the compiler-rt submodule in the compiler-builtins repo though.

Again, taking a look at the rust compiler-rt sources, this would probably not have been strictly necessary, since the affected powi intrinsic seems to have already been ported to rust (yay!).


Note that, as far as I can tell, actually implementing the __aeabi_ intrinsics correctly in rust for arm hard float targets is currently impossible, since I don't see a way to force the rust compiler to use the soft float calling convention on a hard float target.

@alexcrichton
Copy link
Member

Ok, current steps for dealing with ARM:

I think that's it, right? Perhaps we can go ahead and update to use extern "aapcs" everywhere to move forward with this?

@japaric I think one thing that'll be very useful is to run your smoke repo on this PR. It's helped detect a number of compiler-rt regressions in the past! I suspect there's at least one non-x86 regression waiting for us in this PR...

Do you know if it'd be possible to run smoke against this PR ahead of time? Or perhaps just the nightly after landing? Presumably if everything goes wrong we can just revert.

Also it looks like cargo vendor needs to be rerun due to the error on Travis

@japaric
Copy link
Member Author

japaric commented Nov 17, 2016

Do you know if it'd be possible to run smoke against this PR ahead of time?

If you can hand me binary releases (that include this change) of rustc + cross-std for all the targets that smoke tests then, yeah, it should be possible. Otherwise, trying to bootstrap rustc inside the Travis workers will probably timeout.

Or perhaps just the nightly after landing?

This would be the easiest thing to do.

@alexcrichton
Copy link
Member

Ok, sounds good to me

@bors
Copy link
Contributor

bors commented Nov 24, 2016

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

@est31
Copy link
Member

est31 commented Dec 26, 2016

I think the COPYRIGHT file needs to be updated as well, it mentions src/compiler-rt.

@alexcrichton
Copy link
Member

@japaric triage ping on this, do you recall the next steps here?

@est31
Copy link
Member

est31 commented Jan 17, 2017

@alexcrichton I know that at least rust-lang/compiler-builtins#133 blocks this PR, as using the C code is not enough as it only works on 64 bit non windows platforms (same reason as why the i128 pr had to put them into the crate the first place).
Plus then we also need i128 float conversion, but currently the crate doesn't have general float conversion, so I didn't want to make the PR introduce it.

Disclaimer though, I'm not 100% sure the PR I've linked above works if its fully integrated into the compiler (as I couldn't test the integration without doing it myself...).

@alexcrichton
Copy link
Member

@est31 thanks! Makes sense to me.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2017
@japaric
Copy link
Member Author

japaric commented Feb 3, 2017

triage ping on this, do you recall the next steps here?

I've updated the integration metabug in rust-lang/compiler-builtins#66. I'm going to close this PR until all the TODO items in that list are done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants