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

use the simde header library for greater compatibility #251

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mr-c
Copy link

@mr-c mr-c commented Jun 20, 2020

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
Copy link
Author

mr-c commented Sep 4, 2020

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

@mr-c
Copy link
Author

mr-c commented Jan 22, 2021

Dear @infphilo , can you review this PR?

@parkchanhee
Copy link
Collaborator

parkchanhee commented Mar 23, 2021

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

@outpaddling
Copy link

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
Copy link

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
Copy link

martin-g commented Feb 15, 2023

+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
Copy link

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 added a commit to martin-g/hisat2 that referenced this pull request Feb 15, 2023
This is needed for DaehwanKimLab#251
where 'simde' is added as a Git submodule

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g
Copy link

@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
Copy link

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 pushed a commit to mr-c/hisat2 that referenced this pull request Feb 15, 2023
This is needed for DaehwanKimLab#251
where 'simde' is added as a Git submodule

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
mr-c pushed a commit to mr-c/hisat2 that referenced this pull request Feb 15, 2023
This is needed for DaehwanKimLab#251
where 'simde' is added as a Git submodule

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
mr-c pushed a commit to mr-c/hisat2 that referenced this pull request Feb 15, 2023
This is needed for DaehwanKimLab#251
where 'simde' is added as a Git submodule

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@mr-c
Copy link
Author

mr-c commented Feb 15, 2023

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

@martin-g
Copy link

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

@Yikun
Copy link

Yikun commented Mar 9, 2023

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

@martin-g
Copy link

Friendly ping!

@stkgo
Copy link

stkgo commented Aug 22, 2024

Are there any plans to merge this change?

dslarm added a commit to dslarm/bioconda-recipes that referenced this pull request Nov 27, 2024
mencian added a commit to bioconda/bioconda-recipes that referenced this pull request Nov 27, 2024
* Bump build number, will this bump a rebuild on linux-aarch64?

* Add make -j

* Add support for linux-aarch64 and osx-arm64.
With thanks to @mr-c for DaehwanKimLab/hisat2#251

* Add run_exports, bump build number.

* Update meta.yaml

---------

Co-authored-by: Joshua Zhuang <[email protected]>
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.

Is this software can support aarch64(arm64) platform Support for non-amd64 architectures
6 participants