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

Make HAVE_ATTRIBUTE_TARGET check also check that SSSE3 intrinsics work #1886

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

jmarshall
Copy link
Member

We released a new pysam based on HTSlib et al 1.21 last week, and now #1838 has been re-reported as pysam-developers/pysam#1327. So that nobody gets more support requests, it would be good to fix it so that the simd.c code is disabled for these ancient compilers.

Playing with the Compiler Explorer, I confirmed that GCC ≤ 4.8.5 cannot compile simd.c, while GCC ≥ 4.9.0 can. I considered HTS_GCC_AT_LEAST(4,9) but realised that the existing HAVE_SSSE3 appears to be exactly what is needed.

However further Compiler Exploration showed that GCC ≤ 4.8.5 can compile these intrinsics with -mssse3 but not with merely __attribute__((target("ssse3"))), which fails to enable the intrinsics. Hence the right approach is to actually use some intrinsics in the configure test as @daviesrob suggested in #1838 (comment). This PR does that.

GCC 4.8.5 and earlier accept SSSE3 intrinsics with -mssse3; these
compiler versions also accept __attribute__((target("ssse3"))) but
the attribute fails to enable compilation of the intrinsics.

Refactor the check to HAVE_ATTRIBUTE_TARGET_SSSE3, and check both
the attribute and that the SSSE3 intrinsics used (in simd.c) are
in fact compilable in a function with the attribute. Fixes samtools#1838.
jmarshall added a commit to pysam-developers/pysam that referenced this pull request Feb 10, 2025
Apply PR samtools/htslib#1886. GCC 4.8.5 and earlier accept SSSE3
intrinsics with -mssse3; these compiler versions also accept
__attribute__((target("ssse3"))) but the attribute fails to enable
compilation of the intrinsics.
@daviesrob daviesrob self-assigned this Feb 13, 2025
@daviesrob
Copy link
Member

Thanks, I've confirmed that this works with the ancient gcc on Centos 7. I'll also try to keep my centos 7 VM alive to check for any more regressions.

@daviesrob daviesrob merged commit 1682e5e into samtools:develop Feb 13, 2025
9 checks passed
@jmarshall jmarshall deleted the ssse3-configure branch February 13, 2025 18:50
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.

2 participants