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

Remove handcrafted MMX code #4804

Closed
wants to merge 1 commit into from

Conversation

mstembera
Copy link
Contributor

This removes the handcrafted MMX code as discussed here #4786 (comment)

It also fixes a Makefile bug (line 157) where we claimed sse support for x86-32 but didn't actually enable it.

Finally I tried making it more explicit that we have two different kinds of "architecture support". In some cases we have custom handcrafted code using intrinsics. In other cases we simply enable the appropriate -m compiler flag allowing the compiler to emit advanced instructions on its own.

As @vondele pointed out maybe some CI stuff should be looked at.

No functional change
bench: 1246812

@Fanael
Copy link
Contributor

Fanael commented Sep 28, 2023

vec_cleanup in src/nnue/nnue_feature_transformer.h should be removed entirely as well, it was only ever needed for MMX, all other vector ISAs have empty implementation.

(At assembly level, AVX does require cleanup as well, but the compiler takes care of inserting vzerouppers for us in function epilogues where necessary, so we don't have to worry about it at source level.)


It also fixes a Makefile bug (line 157) where we claimed sse support for x86-32 but didn't actually enable it.

That was done deliberately in commit e178a09, the corresponding messages were just not updated.

@mstembera
Copy link
Contributor Author

@Fanael Thank you for your comments. I have updated the PR based on them.

No functional change
bench: 1453057
@vondele vondele added the to be merged Will be merged shortly label Oct 8, 2023
@vondele vondele closed this in 8a91295 Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants