-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: add s390x architecture support #1214
Conversation
44ae53d
to
699620c
Compare
The following lines should be added to the file 'helio/cmake/internal.cmake' to compile the project to s390x architecture:
|
src/core/detail/bitpacking.cc
Outdated
|
||
int error_mask = 0; | ||
for (int i = 0; i < 16; i++) { | ||
if (has_error[i]) { |
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.
hey, have you tested this function?
according to the docs https://doc.rust-lang.org/beta/core/arch/x86_64/fn._mm_movemask_epi8.html
_mm_movemask_epi8 returns the mask of msb bits (0x80). you do something else, IMHO.
Also, if you use scalar code here, there is no point in duplicating exactly the same logic really.
for (int i = 0; i < 16; i++) {
if (has_error[i] & 0x80)
return false;
}
b69f880
to
40e785c
Compare
Signed-off-by: iko1 <[email protected]>
Signed-off-by: iko1 <[email protected]>
Signed-off-by: iko1 <[email protected]>
Signed-off-by: iko1 <[email protected]>
40e785c
to
516c393
Compare
Signed-off-by: iko1 <[email protected]>
This reverts commit 6cc5d8a.
Signed-off-by: iko1 <[email protected]>
src/core/dash_internal.h
Outdated
#ifdef __s390x__ | ||
#include <vecintrin.h> | ||
#else | ||
#include "core/sse_port.h" | ||
#endif |
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.
May I suggest modifying sse_port.h
to #include <vecintrin.h>
instead of relying on the files including it?
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'm sure after the change you suggested, something will break in the tests, so I suggest taking your suggestion to another PR.
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 feel like I need to explain my proposal, which really is a minor one:
Currently there are 2 files that #include "core/sse_port.h"
. In your PR, you are first checking if the platform is s390x, and if so you do not include sse_port.h
but include <vecintrin.h>
instead. My proposal is to change sse_port.h
to check which platform it is, and #include <vecintrin.h>
if relevant.
This is a compilation change, so as long as it builds on all platforms it should not affect tests, right?
Is there anything that I'm missing?
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.
Done.
The filename 'sse_port.h' is misleading because SSE is x86 technology. Can I rename the filename to 'simd_port.h'?
571814c
to
f478ed6
Compare
Signed-off-by: iko1 <[email protected]>
f478ed6
to
acbe2e2
Compare
Signed-off-by: iko1 <[email protected]>
Thanks for your contribution! |
Fixes #939