Skip to content

round: Abolished the CPU rounding mode change and modified it to a different logic. #13435

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 2 commits into from
Mar 8, 2024

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Feb 20, 2024

This doesn't happen with llvm 15.0.7, but I expect there are many users using 15.0.0, so I fixed it.
The llvm that comes with MacOS from the beginning may be 15.0.0, and to use 15.0.7 we need to install it from homebrew.

When I compared the assembly code, the one using llvm 15.0.0 was calculating places > 0 ? value * exponent : value / exponent before the CPU round mode change, so I made fix like this.

failed test:
https://github.com/php/php-src/actions/runs/7945635002/job/21692533874

testing in my repo:
https://github.com/SakiTakamachi/php-src/actions/runs/7968042924/job/21751687740


#pragma STDC FENV_ACCESS ON is included in C99, but many compilers do not support it.
Based on that, I decided that the CPU rounding mode change feature should not be used even if it existed, and I modified it to a different logic that does not use it.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

This code looks very broken. If you want to change the default floating-point environment in C, you must use #pragma STDC FENV_ACCESS ON, otherwise the compiler can and will assume the default environment.

@SakiTakamachi
Copy link
Member Author

@nikic
Thank you! I didn't know this. My code looks like UB.

I'll fix it.

@SakiTakamachi SakiTakamachi changed the title Fixed to avoid incorrect optimization with llvm 15.0.0 round: Abolished the CPU rounding mode change and modified it to a different logic. Feb 20, 2024
@SakiTakamachi
Copy link
Member Author

@nikic
Many compilers do not support #pragma STDC FENV_ACCESS ON, so I modified the logic to not change the CPU rounding mode.

@SakiTakamachi SakiTakamachi requested a review from nikic February 20, 2024 14:39
@SakiTakamachi
Copy link
Member Author

@bukka

Could you please take a look at this?

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I think it makes sense but didn't spend that much time to think about it. It's for master so should be fine.

@SakiTakamachi SakiTakamachi merged commit 2e50371 into php:master Mar 8, 2024
@SakiTakamachi SakiTakamachi deleted the fix/roung_llvm branch March 8, 2024 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants