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

Refactor and consolidate all SIMD handlers #3352

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Feb 26, 2025

  • We currently have "SIMD handler" code across various header files. To reduce duplication and make things more robust, I wanted to see all SIMD related common functionalities in one file. We already have simd_shared.h, but stuff here was pointlessly duplicated across various headers, and this header was not being used everywhere either. This PR fixes that.
  • The strategy of "define an avx2 function to error at runtime when called if compile time support is missing" was a bad idea IMO (and I am probably partly to blame here for putting some code like this). This is a situation that can quite easily prevent right at compile time, by guarding the function declaration/implementation.
  • The "new strategy" I want to introduce via this PR is having the runtime check also serve as the compile time check. I implemented this via a macro that can be tested at compile time with an ifdef, and then at runtime is just the underlying SDL SIMD feature check function. Why? This ensures that we don't call the runtime check when compile time support is missing, even accidentally. Doing so would now result in a compile time error after this PR (instead of runtime error with the current code). This solves the issue I mentioned in the above point.
  • Added a new macro PG_DISABLE_SIMD that can be set (in one place only) to disable all SIMD at compile time.

How to test this PR? This is probably the hard part of reviewing/approving this PR, but:

  • check that there's no performance regressions in anything SIMD related.
  • go through CI logs and check that there's no "your system supports X but X was not compiled" type warnings that surface import can raise.

@ankith26 ankith26 requested a review from a team as a code owner February 26, 2025 10:09
@ankith26 ankith26 force-pushed the ankith26-simd-cleanup branch from 9014dd0 to a08d483 Compare February 26, 2025 10:26
@ankith26 ankith26 marked this pull request as draft February 26, 2025 11:10
@ankith26
Copy link
Member Author

Making this PR a draft because it needs more thought and testing from my side, I may have overlooked a thing or two

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.

1 participant