search

BenLangmead / bowtie2

A fast and sensitive gapped read aligner
GNU General Public License v3.0
664 stars 158 forks source link

Conditionally enable AVX2 256-bit instructions #394

Closed sfiligoi closed 2 years ago

sfiligoi commented 2 years ago

Most recent CPUs support AVX2 256-bit vector instructions, but SwAligner was limited to 128-bit SSE2. With the change, the SwAligner method can obtain the same result with half the instructions.

Note: The AVX2 code path is still disabled by default, but can be enabled by setting the SSE_AVX2 env variable at compile time.

sfiligoi commented 2 years ago

@ch4rr0 As usual, looks like the test-random job hit the wallclock limit; I don't see any errors. The code should be ready for review.

sfiligoi commented 2 years ago

@ch4rr0 I took the conservative approach of not enabling the AVX2 code by default. After this is accepted and merged, we should discuss if and how to make it part of the default distribution binary.

ch4rr0 commented 2 years ago

@ch4rr0 As usual, looks like the test-random job hit the wallclock limit; I don't see any errors.

The code should be ready for review.

Hello, I'm going to separate the simple and random tests into separate workflows. Hopefully it solves this issue.

ch4rr0 commented 2 years ago

@ch4rr0 I took the conservative approach of not enabling the AVX2 code by default.

After this is accepted and merged, we should discuss if and how to make it part of the default distribution binary.

Sounds good.

sfiligoi commented 2 years ago

@ch4rr0 As usual, looks like the test-random job hit the wallclock limit; I don't see any errors. The code should be ready for review.

Hello, I'm going to separate the simple and random tests into separate workflows. Hopefully it solves this issue.

Note: I already separate the two. But I do re-compile, too. Let me try to just reuse the binaries.

sfiligoi commented 2 years ago

@ch4rr0 As usual, looks like the test-random job hit the wallclock limit; I don't see any errors. The code should be ready for review. FYI: The error is real, the error message is just hidden deep in the text. Working on a fix.

sfiligoi commented 2 years ago

@ch4rr0 Success. All tests are passing now.

sfiligoi commented 2 years ago

@ch4rr0 Anything else that needs changing before it can be merged?

sfiligoi commented 2 years ago

Just a reminder that I do not have write privileges, and someone else needs to either merge or reject the PR.

ch4rr0 commented 2 years ago

Hello @sfiligoi, I was on vacation hence the lapse in communication. Ben and I will be reviewing the changes. We appreciate the time spent putting this PR together.