-
Notifications
You must be signed in to change notification settings - Fork 442
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
Bundled htscodecs build failure on ARM #1450
Comments
Building the avx2 etc on ARM isn't a problem as they have What's not ideal is the lack of any ARM specific CI in htslib when we have ARM specific code being used. This was added to htscodecs itself, but htslib uses its own build system and I didn't spot this as a problem when reviewing things. We really need to improve Htslib's cirrus.ci here. It's easy enough just to copy over the config from htscodecs. |
Even building the #ifdefed-out avx2 etc translation units unnecessarily doesn't work completely fine:
I don't know if there are platforms on which this would cause |
I did think about that, but this seemed like the far easier solution given all the platforms we've tried it on so far work just fine. Cross that bridge when (if!) we ever come to it. |
It's so easy to do, it might as well be done right and forestall any bug reports about the warnings. Anyway, the point of this issue is that a bundled htscodecs build currently does actually fail on ARM due to rANS_static32x16pr_neon.o not being built at all. (And to fix that, one might as well do the right thing anyway.) |
A simple solution to the |
Add test compilations to detect ARM Neon support to configure.ac and hts_probe_cc.sh. If compiler support is present, add rANS_static32x16pr_neon.c to $(HTSCODECS_SOURCES) in htscodecs_bundled.mk. Fixes samtools#1450. In htscodecs_bundled.mk, only add rANS_static32x16pr_avx2.c et al to $(HTSCODECS_SOURCES) if the respective AVX2, AVX512, SSE4 support is present. As building these files already uses GNU Make-specific constructs and the $(HTS_CFLAGS_AVX2) variables are either empty or an option string, this is easily achieved via `$(if $(HTS_CFLAGS_AVX2),...)`. There is no compiler flag required for Neon, so invent HTS_HAVE_NEON and use it to control building rANS_static32x16pr_neon.c without adding any bespoke compilation options for it.
Fixed by #1451; as building these SIMD files already uses GNU Make-specific constructs, omitting the unneeded translation units is even more trivial than it would otherwise be. |
Add test compilations to detect ARM Neon support to configure.ac and hts_probe_cc.sh. If compiler support is present, add rANS_static32x16pr_neon.c to $(HTSCODECS_SOURCES) in htscodecs_bundled.mk. Fixes #1450. In htscodecs_bundled.mk, only add rANS_static32x16pr_avx2.c et al to $(HTSCODECS_SOURCES) if the respective AVX2, AVX512, SSE4 support is present. As building these files already uses GNU Make-specific constructs and the $(HTS_CFLAGS_AVX2) variables are either empty or an option string, this is easily achieved via `$(if $(HTS_CFLAGS_AVX2),...)`. There is no compiler flag required for Neon, so invent HTS_HAVE_NEON and use it to control building rANS_static32x16pr_neon.c without adding any bespoke compilation options for it.
Thanks for the fix. Hopefully builds on new Macs will work a bit better now. |
Or, in my case, a build on a Raspberry Pi… |
On current develop on an ARM platform (specifically aarch64 Debian GNU/Linux 11 (bullseye)):
Note that rANS_static32x16pr_avx2.o et al are built and included in libhts.a but rANS_static32x16pr_neon.o is not. It would appear that htscodecs_bundled.mk needs to be able to be configured as appropriate for the host platform.
Note also that the configure script probes unnecessarily for Intel compiler target options, but does not probe for any ARM-related target options (if any are indeed needed). So the configure script could also usefully check
$basic_host
etc and do this target probing as appropriate for the target host.The text was updated successfully, but these errors were encountered: