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

sse2neon -> SIMDe and added non-SIMD version #597

Merged
merged 6 commits into from
Apr 26, 2020
Merged

Conversation

mbrcic
Copy link
Contributor

@mbrcic mbrcic commented Apr 20, 2020

Substituted sse2neon by SIMDe library to convert SSE2 to ARM Neon.
Added option to build non-SIMD version of minimap2 using SIMDe.

SIMDe is added as a git submodule.

Makefile Outdated
CPPFLAGS= -DHAVE_KALLOC
INCLUDES=
OBJS= kthread.o kalloc.o misc.o bseq.o sketch.o sdust.o options.o index.o chain.o align.o hit.o map.o format.o pe.o esterr.o splitidx.o ksw2_ll_sse.o
INCLUDES= -Ilib/simde/simde
Copy link
Contributor

@junaruga junaruga Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be -Ilib/simde rather than -Ilib/simde/simde, modifying the loaded header paths. Because for example #include <simde/x86/sse2.h> is the right call by the simde system library (such as libsimde-dev in Debian) or simde-devel in Fedora and EPEL (CentOS).

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've changed to this in the following commit.

@junaruga
Copy link
Contributor

I really appreciate this PR. Maybe we can try to run the code later on Travis ppc64le and s390x.

Makefile Outdated
@@ -1,26 +1,30 @@
CFLAGS= -g -Wall -O2 -Wc++-compat #-Wextra
CFLAGS= -g -Wall -O2 -Wc++-compat #-Wextra
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary difference. 1 space here.

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've removed the space in the following commit.

@lh3
Copy link
Owner

lh3 commented Apr 20, 2020

Thanks. Could you make SIMDe an option, not the default? For example:

make simde=1

@mbrcic
Copy link
Contributor Author

mbrcic commented Apr 20, 2020

All the comments so far have been implemented.

#include <emmintrin.h>
#else
#include <simde/x86/sse2.h>
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

I think this part could like like this. And other parts are as well.
Because it's natural for me such as "if simde is used, include the simde header, otherwise include the default header.".

But let's wait @lh3 's response.

#ifdef USE_SIMDE
#include <simde/x86/sse2.h>
#else
#include <emmintrin.h>
#endif

Copy link
Contributor Author

@mbrcic mbrcic Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there was no other response, I have changed C preprocessor conditionals to the above stated form.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It looks good to me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I personally always use #define SIMDE_ENABLE_NATIVE_ALIASES + #include <simde/x86/sse2.h> as the SIMDe library has fallback that get compiled away to the plain intrinsic calls when on x86 with the regular compilation flag (-msse4.1, etc).

@junaruga
Copy link
Contributor

I tested your PR on my forked repository adding new use cases tests in .travis.yml.
junaruga@7c0f286

simde=1 test on arch: arm64 is failed with the following error.
https://travis-ci.org/github/junaruga/minimap2/jobs/678368841#L162

gcc -c -g -Wall -O2 -Wc++-compat -DSIMDE_ENABLE_NATIVE_ALIASES -DUSE_SIMDE -msse2 -DHAVE_KALLOC -Ilib/simde ksw2_ll_sse.c -o ksw2_ll_sse.o
gcc: error: unrecognized command line option ‘-msse2’

Perhaps it might be good to add the tests in `.travis.yml in this PR, to guarantee the modification.
When Heng wants to modify your commits to adjust, he might do cherry-pick your commit modifying it.

@mbrcic
Copy link
Contributor Author

mbrcic commented Apr 22, 2020

simde=1 just enables SIMDe, but for current Makefile you have to enable also arm_neon=1 for arm and aarch64=1 for aarch64. I have tested with such setting on arm64.

The failed test used SIMDe, but pushed compiling with sse2 flag even though code contains no SSE2 code. arm64 compilers have no such flag.

@mbrcic
Copy link
Contributor Author

mbrcic commented Apr 22, 2020

simde=1 just enables SIMDe, but for current Makefile you have to enable also arm_neon=1 for arm and aarch64=1 for aarch64. I have tested with such setting on arm64.

The failed test used SIMDe, but pushed compiling with sse2 flag even though code contains no SSE2 code. arm64 compilers have no such flag.

Another option is to put just activation of simde=1 and put into all compiler calls in Makefile -march=native. Then SIMDe and gcc would work for all combinations by auto-detect, including x86 / x86_64. But, this is high-level decision that I did not feel I could make.

@junaruga
Copy link
Contributor

Okay. In my humble opinion, I would prefer that make simde=1 works on non-x86_64 environment without arm_neon=1 or aarch64=1 if it is no problem for the performance and maintainability.
But I am just a contributor. So, let's wait Heng's response.

By the way, here is simde's .travis.yml. We can see some compiler flags tested on non-x86_64 environments.

@lh3 lh3 merged commit 66db9da into lh3:master Apr 26, 2020
@lh3
Copy link
Owner

lh3 commented Apr 26, 2020

@mbrcic In the end, I decide to create a new Makefile.simde for simde; otherwise Makefile would become too complex. Makefile.simde doesn't support CPU dispatch for simplicity. CPU dispatch is very rarely needed these days anyway.

@junaruga Currently minimap2 doesn't have test for Makefile.simde.

@junaruga
Copy link
Contributor

@lh3 I have a suggestion. @mbrcic Could you send a pull-request to add a arch: arm64 case using make -f Makfile.simde updating .travis.yml, to test Makfile.simde and to show the reproductive command steps to minimap2 developers or users? I think it's useful.

@mbrcic
Copy link
Contributor Author

mbrcic commented May 11, 2020

Hi @junaruga , I've just made a PR with the requested changes.

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.

4 participants