DaehwanKimLab / hisat2

Graph-based alignment (Hierarchical Graph FM index)
GNU General Public License v3.0
464 stars 113 forks source link

use the simde header library for greater compatibility #251

Open mr-c opened 4 years ago

mr-c commented 4 years ago

This patch from Debian allows building and running hisat2 on non-x86 systems like arm64, ppc64el, s390x, and more: https://buildd.debian.org/status/package.php?p=hisat2

Closes #244 Closes #67

mr-c commented 3 years ago

Dear @parkchanhee , is there anything I can do to get this PR merged? Thanks!

mr-c commented 3 years ago

Dear @infphilo , can you review this PR?

parkchanhee commented 3 years ago

@mr-c Thank you for nice PR. I'll test it on non-x86/x64 arch

outpaddling commented 2 years ago

This is awesome, thanks for posting.

I think you might have a precedence issue, though. The following evaluates to true if i386 is defined regardless of GNUC:

#elif defined(__GNUC__) && defined(__x86_64__) || defined(__i386__)

Should it be the following instead?

#elif defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__))

outpaddling commented 2 years ago

I just updated the FreeBSD port to include support for aarch64 and powerpc64.

In addition to the above patch set, the following changes were also needed:

Remove -DPOPCNT_CAPABILITY Add -fsigned-char since clang defaults to unsigned on aarch64

martin-g commented 1 year ago

+1 for adding support for Linux ARM64! We need it for the Bioconductor project: https://yikun.github.io/bioconductor-0208/report/Rhisat2/kunpeng1-buildsrc.html I will test this PR and send PRs for adding CI/testing for x86_64 and aarch64!

martin-g commented 1 year ago

I'd also suggest the following change in the Makefile to not add -m64 and -msse2 for non-x86_64 machines:

diff --git Makefile Makefile
index b839981..f9a6ce2 100644
--- Makefile
+++ Makefile
@@ -152,9 +152,10 @@ HISAT2_REPEAT_CPPS_MAIN = $(REPEAT_CPPS) $(BUILD_CPPS) hisat2_repeat_main.cpp
 SEARCH_FRAGMENTS = $(wildcard search_*_phase*.c)
 VERSION = $(shell cat VERSION)

+ARCH=$(shell uname -m)
 # Convert BITS=?? to a -m flag
 BITS=32
-ifeq (x86_64,$(shell uname -m))
+ifeq (x86_64,$(ARCH))
 BITS=64
 endif
 # msys will always be 32 bit so look at the cpu arch instead.
@@ -165,14 +166,16 @@ ifneq (,$(findstring AMD64,$(PROCESSOR_ARCHITEW6432)))
 endif
 BITS_FLAG =

-ifeq (32,$(BITS))
-       BITS_FLAG = -m32
-endif
+ifeq (x86_64,$(ARCH))
+       ifeq (32,$(BITS))
+               BITS_FLAG = -m32
+       endif

-ifeq (64,$(BITS))
-       BITS_FLAG = -m64
+       ifeq (64,$(BITS))
+               BITS_FLAG = -m64
+       endif
+       SSE_FLAG=-msse2
 endif
-SSE_FLAG=-msse2

 DEBUG_FLAGS    = -O0 -g3 $(BITS_FLAG) $(SSE_FLAG)
 DEBUG_DEFS     = -DCOMPILER_OPTIONS="\"$(DEBUG_FLAGS) $(EXTRA_FLAGS)\""
martin-g commented 1 year ago

@mr-c You may also want to incorporate the changes I suggest with https://github.com/DaehwanKimLab/hisat2/pull/408/files - CI for Linux x86_64 and aarch64. It will make it easier for the repo maintainers to see that x86_64 still builds and there is no problem with Linux aarch64

martin-g commented 1 year ago

I may be doing something wrong but the build of this branch fails on my system (Linux openEuler 22.03 LTS ARM64):

...
/tmp/ccGNrEl1.s: Assembler messages:
/tmp/ccGNrEl1.s:75869: Error: unknown mnemonic `popcntq' -- `popcntq x18,x2'
/tmp/ccGNrEl1.s:75919: Error: unknown mnemonic `popcntq' -- `popcntq x17,x18'
/tmp/ccGNrEl1.s:75989: Error: unknown mnemonic `popcntq' -- `popcntq x15,x28'
/tmp/ccGNrEl1.s:76041: Error: unknown mnemonic `popcntq' -- `popcntq x18,x16'
...

Full logs: hisat2-linux-arm64.txt

The steps I did: 1) gh pr checkout 251 - to checkout this PR 2) git submodule update --init --recursive - to fetch simde submodule 3) make -j all

mr-c commented 1 year ago

Thank you @martin-g for the feedback! I pushed some changes that should fix that

martin-g commented 1 year ago

I confirm that now the project builds fine on Linux ARM64! Thank you, @mr-c !

Yikun commented 1 year ago

I also validated it in my Linux aarch64, it works, so would you mind taking a look on this? @parkchanhee

martin-g commented 1 year ago

Friendly ping!