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 dispatch rather than rely on separate compile commands for SSSE3 bam parser #131

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

rhpvorderman
Copy link
Collaborator

See: samtools/htslib#1764

Using dispatch is less flaky than precompiling with SSSE3 instructions. It will work on any machine.

I made the dispatching work properly with GCC. I also used the basic nibble2base function in htslib as a base for the default.

From prior discussion in htslib I learned that deoding the qualities can be easily optimized by the compiler including the tricks to do so.

@rhpvorderman
Copy link
Collaborator Author

@marcelm Friendly reminder ping. This will make the build arguments easier so it has my preference over the current solution of always compiling on with -mssse3 on linux.

src/dnaio/bam.h Outdated
Comment on lines 9 to 10
#ifdef __clang__
#ifdef __has_attribute
Copy link
Owner

Choose a reason for hiding this comment

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

Since the above macro uses #if defined instead of #ifdef, can you do that here as well for consistency? And the two #ifdefs can then also combined into if defined ... && defined.

#endif
#ifndef CLANG_COMPILER_HAS
#define CLANG_COMPILER_HAS(attribute) 0
Copy link
Owner

Choose a reason for hiding this comment

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

This can then be in an #else branch

@rhpvorderman
Copy link
Collaborator Author

Thanks for the review. It is done. Also I found some issues when compiling on ARM64 when this code was incorporated into sequali. I have updated the macros such that they will work now on ARM64 as well.

I am not very fond of all this compiler checking macors, but I see no better way alas.

@marcelm marcelm merged commit 94e09d7 into marcelm:main Apr 22, 2024
16 checks passed
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