Skip to content

Fix #79595: zend_init_fpu() alters FPU precision #5602

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
Closed
wants to merge 2 commits into from
Closed

Fix #79595: zend_init_fpu() alters FPU precision #5602

wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented May 20, 2020

Just checking an idea of @nikic.

@cmb69
Fix #79595: zend_init_fpu() alters FPU precision
b8f3811 
On startup, PHP deliberately changes the floating point control word to
enforce binary64 format for the calculations for best consistency
across platforms.  However, this is unnessary when compiling under
`__SSE__`, because in this case the x87 instructions are not used.
Therefore, we can skip the modification, which has the benefit that
system libraries are free to work in the mode of their liking.
@cmb69 cmb69 changed the title Don't mess with floating-point control word on SSE architectures Fix #79595: zend_init_fpu() alters FPU precision May 22, 2020
@cmb69 cmb69 marked this pull request as ready for review May 22, 2020 07:37
@cmb69
Copy link
Member Author

cmb69 commented May 22, 2020

I suggest to apply this as bug fix. Maybe we should postpone to master.

@nikic
Copy link
Member

nikic commented May 22, 2020

I'm okay with landing this on 7.4, at least I don't see any obvious complications this would cause.

@cmb69
Copy link
Member Author

cmb69 commented May 22, 2020

@derickr, @petk, is this okay for PHP-7.4?

@derickr
Copy link
Member

derickr commented May 22, 2020 via email

@cmb69 cmb69 added the Bug label May 22, 2020
@cmb69
Copy link
Member Author

cmb69 commented May 22, 2020

Since this change is okay for @derickr, I've applied it as 88dfc47. Thanks!

@cmb69 cmb69 closed this May 22, 2020
@cmb69 cmb69 deleted the cmb/FPCW branch May 22, 2020 13:49
@nikic
Copy link
Member

nikic commented May 22, 2020

There are failures on master due to this change. The issue is that we compile with -msse2, which means that __SSE__ is set, but x87 is still used, because that's controlled by a separate flag -mfpmath=sse.

@nikic
Copy link
Member

nikic commented May 22, 2020

I've temporarily reverted this change. I think the basic approach here is still right, but we need to be smarter about detecting whether the FPU will be used or not. I don't think there is any macro that tells us, so probably the next best thing is to check whether we're on x86_64, which always has SSE2. In that case it's still possible to opt-in to x87 using -mfpmath=387, but there's no reason to ever do that, so I think this is the best approximation we have.

@cmb69 cmb69 restored the cmb/FPCW branch May 22, 2020 15:16
@cmb69
Copy link
Member Author

cmb69 commented May 22, 2020

Thanks, Nikita, I'll have a look.

@cmb69 cmb69 reopened this May 22, 2020
@cmb69 cmb69 mentioned this pull request May 25, 2020
Closed
@cmb69
Copy link
Member Author

cmb69 commented May 25, 2020

Superseded by PR #5621.

@cmb69 cmb69 closed this May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

Assignees
No one assigned
Labels
Bug Waiting on Author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmb69 @nikic @derickr