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

Allow optimization and use fesetround(), fegetround() #642

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

howjmay
Copy link
Collaborator

@howjmay howjmay commented Jul 18, 2024

feat: Allow optimization option in sse2neon.h

Revert the previous restriction that some of the functions are forced
to not be optimized when compiling-time optimization options were
given.
Now it is users' responsibility to ensure the behavior after
optimization.
Shifting the responsibility to the users enables sse2neon the run in
the optimized state in general, but not restricted by some specific
scenarios.

feat: Use fesetround() and fegetround()

Setting/getting rounding mode directly through fpcr with volatile
keyword could be unstable.
Therefore we use the C99 fesetround()/fegetround() here to ensure
the behavior.

closes #648

@howjmay howjmay marked this pull request as ready for review July 18, 2024 15:06
@howjmay howjmay requested review from jserv and marktwtn as code owners July 18, 2024 15:06
@howjmay howjmay marked this pull request as draft July 18, 2024 16:14
@howjmay howjmay mentioned this pull request Jul 25, 2024
Copy link
Member

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase the latest master branch.

@howjmay howjmay force-pushed the test-opt branch 4 times, most recently from f30c404 to 746be68 Compare August 25, 2024 06:58
@howjmay howjmay force-pushed the test-opt branch 6 times, most recently from 6e3fd9a to 0285a2b Compare September 22, 2024 01:41
@howjmay howjmay changed the title refactor: Disable optimization for some intrinsics refactor: Allow optimization and use fesetround(), fegetround() Sep 22, 2024
@howjmay howjmay force-pushed the test-opt branch 3 times, most recently from c917fce to 2b131e7 Compare September 22, 2024 07:31
@howjmay howjmay marked this pull request as ready for review September 22, 2024 07:32
case FE_TOWARDZERO:
return _MM_ROUND_TOWARD_ZERO;
default: // FIXME
return _MM_ROUND_TOWARD_ZERO;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jserv I am not sure whether we have a better value to return in the error case, so I temporarily return _MM_ROUND_TOWARD_ZERO here.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether we have a better value to return in the error case, so I temporarily return _MM_ROUND_TOWARD_ZERO here.

It is reasonable, and you should explain more in comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added comments for explaining the error case

rounding = FE_TOWARDZERO;
break;
default: // FIXME
rounding = FE_TOWARDZERO;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same here

@jserv jserv changed the title refactor: Allow optimization and use fesetround(), fegetround() Allow optimization and use fesetround(), fegetround() Sep 22, 2024
@jserv
Copy link
Member

jserv commented Sep 22, 2024

I would like to ask @jmather-sesi and @alexorlov124 for reviewing.

Copy link
Member

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Remove the label refactor in the git commit messages. The changes were not associated with refactoring. Instead, the commits did change the behavior with compiler optimizations.

Revert the previous restriction that some of the functions are forced
to not be optimized when compiling-time optimization options were
given.

Now it is users' responsibility to ensure the behavior after
optimization.

Shifting the responsibility to the users enables sse2neon the run in
the optimized state in general, but not restricted by some specific
scenarios.
@howjmay
Copy link
Collaborator Author

howjmay commented Oct 6, 2024

I changed to feat labels now

README.md Outdated Show resolved Hide resolved
sse2neon.h Outdated Show resolved Hide resolved
@howjmay howjmay force-pushed the test-opt branch 4 times, most recently from df98efc to 398344f Compare October 8, 2024 16:18
sse2neon.h Outdated Show resolved Hide resolved
Setting/getting rounding mode directly through fpcr with volatile
keyword could be unstable.
Therefore we use the C99 fesetround()/fegetround() here to ensure
the behavior.
@jserv jserv merged commit d1562a7 into DLTcollab:master Oct 8, 2024
16 checks passed
@jserv
Copy link
Member

jserv commented Oct 8, 2024

Thank @howjmay for contributing!

@howjmay
Copy link
Collaborator Author

howjmay commented Oct 8, 2024

thanks for reviewing

@howjmay howjmay deleted the test-opt branch October 8, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizations disabled in _MM_SET_ROUNDING_MODE
2 participants