Skip to content

Remove the rem_float intrinsics #48685

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

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

gbaraldi
Copy link
Member

Since #47501 we have native versions of the rem functions, but the original PR didn't change the fastmath version. It turns out that OpenLibm's fastmath version was already slower than the non fast one, so add a fast version of it and remove the intrinsics.

@gbaraldi
Copy link
Member Author

@nanosoldier runtests()

@ViralBShah
Copy link
Member

ViralBShah commented Feb 16, 2023

The win64 test had some failures and then eventually timed out - but the failure is not being detected right in the CI.

https://buildkite.com/julialang/julia-master/builds/21178#01865601-bb06-4472-910d-f58e0dc9c443/904-1109

cc @DilumAluthge

@DilumAluthge
Copy link
Member

Hmmm. In this case, it seems like the timeout worked correctly, right? The win64 tests timed out at 3 hours, so Buildkite killed the job and exited with status code 1. Then Gabriel retried the failed job; the retry succeeded.

@DilumAluthge
Copy link
Member

However, on Viral's other PR, Windows timed out, and Buildkite exited with status code zero, which is obviously a big problem.

@staticfloat has sent a support ticket into Buildkite; we're waiting to hear back.

@ViralBShah
Copy link
Member

Oh I didn't realize that. I see the successful job next.

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@gbaraldi
Copy link
Member Author

It seems the only package that used the intrinsic is Unitful. They seem to use the intrinsics directly which seems a bit odd.

@ViralBShah
Copy link
Member

ViralBShah commented Feb 16, 2023

I suppose that since the Unitful.jl PR is merged, this PR should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants