-
Notifications
You must be signed in to change notification settings - Fork 56
Move NEON SIMD helpers from simd.hpp to neon.hpp
#630
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
Move NEON SIMD helpers from simd.hpp to neon.hpp
#630
Conversation
|
@tigercosmos @KHLee529 this fix is still work in progress and does not yet pass CI, but I am curious why we got the build error in the code base (or is it in my environment)? |
|
As what I found from simply look into code, |
As for this question, in short, I prefer putting it in When I designed the SIMD library, I treated
flowchart TD
A[SIMD::check_between] --> C{detect SIMD support}
C --> |support neon| N[neon::check_between]
C --> |support AVX512| X[avx512::check_between]
C --> |not support any SIMD feature| G[generic::check_between]
So In another way of thinking, if the @yungyuc @tigercosmos This is my thought to this SIMD library. Please comment or suggest your opinion. |
|
I've tested the latest master locally on Ubuntu and macOS, and I haven't encountered any compilation errors. However, I agree with @KHLee529's solution. |
177bdc5 to
d3d3d23
Compare
| #endif // NDEBUG | ||
|
|
||
| // Get the recommended memory alignment for SIMD operations based on the detected SIMD instruction set. | ||
| inline constexpr size_t get_recommended_alignment() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think get_recommended_alignment should be an implementation detail in this early stage of development. If we find it is a generally useful helper, then take it outside detail in the future.
| { | ||
| #if defined(__aarch64__) || defined(__arm__) | ||
| return 16; | ||
| #elif defined(__AVX512F__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper contains non-NEON conditionals that should be refactored in the future.
| { | ||
| using namespace detail; | ||
| switch (detect_simd()) | ||
| switch (detail::detect_simd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not using namespace detail, or defeat part of the purpose of the namespace detail. I created issue #631 to get rid of the misusing.
|
Thank you, @tigercosmos @KHLee529 . I am following @KHLee529 recommendation to move the NEON-specific code to PR is ready for review. The code passed CI in my fork. |
simd.hpp to neon.hpp
| return 16; | ||
| #elif defined(__AVX512F__) | ||
| return 64; | ||
| #elif defined(__AVX__) || defined(__AVX2__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we put this function in NEON, then we don't need these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK. The non-NEON conditional code is OK as a hint for a need for refactoring.
To make it clear, I added a comment for TODO 3 lines above.
|
|
Without the patch, I got build error on my local macos 26:
It seems to come from PR #628 , but I do not know why CI did not catch it.
@KHLee529 is
simd_generic.hppa good location for the helpers? I am not sure about it.