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

feat: add s390x architecture support #1214

Merged
merged 9 commits into from
Jun 18, 2023

Conversation

iko1
Copy link
Contributor

@iko1 iko1 commented May 14, 2023

Fixes #939

src/core/detail/bitpacking.cc Show resolved Hide resolved
src/core/detail/bitpacking.cc Show resolved Hide resolved
src/core/detail/bitpacking.cc Outdated Show resolved Hide resolved
@iko1 iko1 force-pushed the feature/add-s390x-support branch from 44ae53d to 699620c Compare May 27, 2023 12:18
@iko1 iko1 marked this pull request as ready for review May 27, 2023 13:17
@iko1
Copy link
Contributor Author

iko1 commented May 27, 2023

The following lines should be added to the file 'helio/cmake/internal.cmake' to compile the project to s390x architecture:

elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL "s390x")
set(MARCH_OPT "-march=native")
set(COMPILE_OPTS "${COMPILE_OPTS} -mzvector")

@iko1 iko1 requested a review from romange May 28, 2023 14:02

int error_mask = 0;
for (int i = 0; i < 16; i++) {
if (has_error[i]) {
Copy link
Collaborator

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;
}

@iko1 iko1 force-pushed the feature/add-s390x-support branch from b69f880 to 40e785c Compare June 13, 2023 20:39
@iko1 iko1 force-pushed the feature/add-s390x-support branch from 40e785c to 516c393 Compare June 17, 2023 11:40
Comment on lines 16 to 20
#ifdef __s390x__
#include <vecintrin.h>
#else
#include "core/sse_port.h"
#endif
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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'?

@iko1 iko1 force-pushed the feature/add-s390x-support branch 2 times, most recently from 571814c to f478ed6 Compare June 17, 2023 20:53
Signed-off-by: iko1 <me@remotecpp.dev>
@iko1 iko1 force-pushed the feature/add-s390x-support branch from f478ed6 to acbe2e2 Compare June 18, 2023 07:01
@iko1 iko1 requested a review from chakaz June 18, 2023 16:25
@chakaz chakaz merged commit 19d7622 into dragonflydb:main Jun 18, 2023
7 checks passed
@chakaz
Copy link
Collaborator

chakaz commented Jun 18, 2023

Thanks for your contribution!

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.

s390/ibm/linuxone compatibility
3 participants