You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(ext): walk the ungapped mismatch bitmap one set bit at a time - #697
ungapped_walk_mis scores the ungapped diagonal from the mismatch bitmap one mismatch at a time: the HIT path uses it for the committed score and qle, and the TIGHT path for the walk score its band is derived from. It found each next mismatch by rescanning the bitmap from the current position, masking the current word below j and looking at that word and then later ones for the lowest set bit. This PR visits the mismatches in increasing order by walking each word's set bits instead (lowest bit with ctz, then bits &= bits - 1), loading the next word only when the current one has none left, and scores the final run of matches up to N after the loop. Output is byte-identical. The function has no ISA-specific code, so the change applies to every build: x86 at every tier and arm64 NEON alike.
The loop. The words walked are those holding a position below N, capped at the bitmap's size (FP_MIS_NWORDS). ungapped_analyze sets no bit at or past N, so every set bit visited is a real mismatch and the set bits come out in increasing position order. Each mismatch applies the same steps as before: add the run of matches before it, test for a strict new record (the earliest tied position keeps the record), subtract the mismatch penalty, floor at 0, and stop once the score reaches 0.
The final run. After the last mismatch the run of matches up to N is scored once, with the record position N. When the last mismatch is at N - 1 that run is empty and skipped. When the score reaches 0 the walk jumps past it, as the old loop's break did, so the outputs are frozen there as in the per-base walk.
Unchanged. The envelope check (a > 0, b >= 0, h0 > 0, else the per-base walk), the record, floor and stop rules, and the three outputs (score, record position, final value) are as before. BWA_MEM3_DEBUG_UNGAPPED_XCHECK still cross-checks every call against the per-base walk.
Correctness
Unit tests. The existing test compares the run walk with the per-base walk on 4000 random masks (sparse, dense, adjacent mismatches, b = 0). A new test pins the boundaries the random masks reach only by chance: N one short of, at, and one past representative multiples of 64 up to the full bitmap (FP_N_MAX = 512), with no mismatches, every base a mismatch, mismatches at bits 0 and 63 of each word, a mismatch at N - 1 (an empty final run), and a score that reaches 0 with mismatches left in the same word, or on a word's last mismatch with mismatches left only in a later word. It runs five scoring schemes (including b = 0 and h0 = 1) over each layout and asserts that the empty-final-run case and both stop-with-mismatches-left cases were actually reached.
The tests fail when they should. Undercounting the words to walk by one position, so that a last word holding only position N - 1 is skipped, fails the new test, and recording N - 1 instead of N for the final run fails both the new test and the existing ones.
Generated code. The review follow-up in src/ is a single comment in ungapped_ext.h, with its line count and its xassert line unchanged. Every object that includes ungapped_ext.h (bwamem.o and the seven unit-test objects) disassembles identically before and after it on arm64 and on an x86-64 build at the AVX2 tier.
Test suites. Unit suite 618/618 and test/run_unit_tests.sh pass natively on arm64 (bwtSeed lockstep parity 95/95, SMEM lockstep parity 56/56), as do band_cert_identity.sh and the shell lint. A build with -DBWA_MEM3_DEBUG_UNGAPPED_XCHECK passes the unit suite (618/618) and its end-to-end runs below trip no cross-check. On an x86-64 build at the AVX2 tier, the unit suite (597 passed, one AVX-512-only case skipped) and the bwtSeed and SMEM lockstep parity tests (95/95, 56/56) pass.
End to end. These runs are byte-identical to main (excluding @PG) on arm64, on the arm64 cross-check build, and on an x86-64 build at the AVX2 tier: 100k pairs of the WGS slice below against hg38, SE and PE with default options, PE with -d 70, -w 30, --compat=bwa-mem2, -k 10, -k 30, -y 1, -L 0 and --ks-dedup=off, and long reads (300 bp, 1 kbp, 3 kbp).
Timing runs. SAM output was identical to main on every timing run on all three hosts.
Timing
Whole aligner, mem -t 16 on a 5M-pair WGS slice (HG00096, 2x150, hg38), clang 19.1.7, index in shared memory, 6 interleaved reps per build in a seeded shuffled order on the same host. This change was measured on its own against main. Figures are the paired per-rep delta vs main (median, bootstrap 95% CI).
host
wall
CPU (user)
c7i.4xlarge (Sapphire Rapids, AVX-512BW)
−0.15% [−0.26, −0.11]
−0.10% [−0.26, −0.05]
c6a.4xlarge (Zen 3, AVX2)
−0.33% [−1.25, +0.49]
−0.34% [−1.15, +0.51]
c8g.4xlarge (Graviton4, NEON)
−0.32% [−1.52, +0.09]
−0.38% [−1.60, −0.01]
Start-up included: the first 200k pairs of the same slice, -t 16, with the index loaded from the page cache on every run, 12 interleaved reps:
host
wall
CPU (user)
c7i.4xlarge (Sapphire Rapids, AVX-512BW)
−0.18% [−0.53, +0.00]
−0.23% [−0.45, +0.09]
c6a.4xlarge (Zen 3, AVX2)
+0.00% [+0.00, +0.16]
−0.31% [−1.21, −0.01]
c8g.4xlarge (Graviton4, NEON)
−0.61% [−0.92, +0.00]
−0.56% [−1.08, −0.27]
The walk runs on every architecture, so Graviton4 is a measurement here, not a null control. At 5M pairs only Sapphire Rapids excludes zero on both wall and CPU, at about −0.15% and −0.10%. On Zen 3 both intervals cross zero. On Graviton4 the wall interval crosses zero and the CPU interval just excludes it (upper end −0.01%). Every 5M median is negative. At 200k the Zen 3 and Graviton4 CPU intervals exclude zero; the Sapphire Rapids and Graviton4 wall intervals touch zero, the Zen 3 wall median is exactly 0 with its interval touching it, and the Sapphire Rapids CPU interval crosses it. Read the 200k figures with care. A 200k run takes a few seconds, so the timer's 10 ms resolution is 0.3–0.6% of a run and the wall medians sit on that grid. On Sapphire Rapids that batch also ran faster than main for most changes measured alongside this one, so its 200k figure is not this change's own effect; the 5M-pair runs are the figure to go by. No interval excludes zero on the slow side.
Stack
This is the thirteenth PR of a stacked series that reworks SMEM seeding, SA lookup and extension batching. It is based on #696 and should be reviewed after it. The next PR in the series is "perf(ext): probe non-HIT extensions up to 512 bases in the certified rung".
Split out of #656, thanks to @jmschrei for the original work.
Summary by CodeRabbit
Performance
Ungapped alignment walks now process mismatches more efficiently while preserving scoring and endpoint behavior.
Tests
Added boundary coverage comparing two mismatch-walk methods across varied lengths, scoring configurations, and mismatch patterns.
No actionable comments were generated in the recent review.
Walkthrough
ungapped_walk_mis now scans mismatch bitmap words and visits set bits in order. Deterministic tests compare its score, endpoint, and current score with the per-base walk across selected boundary lengths, scoring configurations, and mismatch patterns.
Changes
Ungapped mismatch walk
Layer / File(s)
Summary
Bitmap scan and output comparisons src/ungapped_ext.h, test/unit/test_ungapped_fastpath.cpp
ungapped_walk_mis scans set bits in order, scores match runs, applies mismatch penalties, and stops when the score reaches zero. Tests compare its outputs with the per-base walk across selected boundary cases.
The bitmap scan preserves the scoring behavior supported by the inspected code and boundary tests. No actionable merge-blocking issue was found.
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Public-Repo Hygiene
The changed-file diff and committed message contain no detected object-store URL, cloud identifier, private address, machine-local path, or disallowed named resource. The supplied pull-request body is…
Provide the complete pull-request body and review-comment text, then scan them and the title, commit messages, and changed lines for the prohibited names, URLs, identifiers, hostnames, and local paths.
Passed checks: 7 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title uses the valid perf(ext): Conventional Commits format and identifies the ext component. It accurately describes the bitmap set-bit iteration change and contains no prohibited internal na…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Equivalence Contract Updated
PASS. The PR changes src/ungapped_ext.h, which src/bwamem.cpp includes, so the fast-path can affect alignment records and emitted SAM/BAM. The PR description explicitly states that output is byte-…
Kernel Change Validation Reported
The pull request changes only src/ungapped_ext.h and test/unit/test_ungapped_fastpath.cpp. It does not modify any of the files covered by this check.
Simd Tier Coverage
PASS: The diff changes only scalar mismatch-bit iteration in ungapped_walk_mis and adds scalar unit coverage. It adds no SIMD intrinsic, tier dispatch, or SIMD_WIDTH-dependent buffer change. The e…
Full details: Title check
Explanation
The title uses the valid perf(ext): Conventional Commits format and identifies the ext component. It accurately describes the bitmap set-bit iteration change and contains no prohibited internal names or paths.
Full details: Equivalence Contract Updated
Explanation
PASS. The PR changes src/ungapped_ext.h, which src/bwamem.cpp includes, so the fast-path can affect alignment records and emitted SAM/BAM. The PR description explicitly states that output is byte-identical and describes verification through unit tests, debug cross-checks, disassembly comparison, and end-to-end byte comparisons across arm64 and x86-64 AVX2. The custom check therefore allows this change without an equivalence-doc update.
Full details: Simd Tier Coverage
Explanation
PASS: The diff changes only scalar mismatch-bit iteration in ungapped_walk_mis and adds scalar unit coverage. It adds no SIMD intrinsic, tier dispatch, or SIMD_WIDTH-dependent buffer change. The existing _mm_* and __AVX2__ code in src/ungapped_ext.h is unchanged.
Full details: Public-Repo Hygiene
Explanation
The changed-file diff and committed message contain no detected object-store URL, cloud identifier, private address, machine-local path, or disallowed named resource. The supplied pull-request body is explicitly truncated, and the complete body and review-comment text are unavailable. This check requires scanning all body and review-comment text, so PASS cannot be established.
Resolution
Provide the complete pull-request body and review-comment text, then scan them and the title, commit messages, and changed lines for the prohibited names, URLs, identifiers, hostnames, and local paths.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 028e7b4 and 9014651.
Files selected for processing: 2
src/ungapped_ext.h
test/unit/test_ungapped_fastpath.cpp
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
ungapped_walk_mis found each next mismatch by rescanning the bitmap
from the current position: it masked the current word below j and
looked at it and then later words for the lowest set bit. Visit the
mismatches in increasing order instead by walking each word's set bits
(lowest bit, then bits &= bits - 1), loading the next word only when
this one has none left, and score the final run of matches to N after
the loop. The record, floor and stop rules are unchanged, so the walk
returns the same score, record position and final value; the per-base
walk equivalence test and BWA_MEM3_DEBUG_UNGAPPED_XCHECK check it.
Byte-identical.
A new unit test compares the walk with the per-base walk at the
boundaries the random masks reach only by chance: N one short of, at,
and one past representative multiples of 64 up to the full bitmap
(FP_N_MAX), no mismatches, every base a mismatch, mismatches at bits 0
and 63 of each word and at N - 1 (an empty final run), and a score that
reaches 0 with mismatches left in the same word, or on a word's last
mismatch with mismatches left only in a later word.
Co-authored-by: Jacob Schreiber <jacob.schreiber@umassmed.edu>
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ungapped_walk_misscores the ungapped diagonal from the mismatch bitmap one mismatch at a time: the HIT path uses it for the committed score andqle, and the TIGHT path for the walk score its band is derived from. It found each next mismatch by rescanning the bitmap from the current position, masking the current word belowjand looking at that word and then later ones for the lowest set bit. This PR visits the mismatches in increasing order by walking each word's set bits instead (lowest bit withctz, thenbits &= bits - 1), loading the next word only when the current one has none left, and scores the final run of matches up toNafter the loop. Output is byte-identical. The function has no ISA-specific code, so the change applies to every build: x86 at every tier and arm64 NEON alike.N, capped at the bitmap's size (FP_MIS_NWORDS).ungapped_analyzesets no bit at or pastN, so every set bit visited is a real mismatch and the set bits come out in increasing position order. Each mismatch applies the same steps as before: add the run of matches before it, test for a strict new record (the earliest tied position keeps the record), subtract the mismatch penalty, floor at 0, and stop once the score reaches 0.Nis scored once, with the record positionN. When the last mismatch is atN - 1that run is empty and skipped. When the score reaches 0 the walk jumps past it, as the old loop'sbreakdid, so the outputs are frozen there as in the per-base walk.a > 0,b >= 0,h0 > 0, else the per-base walk), the record, floor and stop rules, and the three outputs (score, record position, final value) are as before.BWA_MEM3_DEBUG_UNGAPPED_XCHECKstill cross-checks every call against the per-base walk.Correctness
b = 0). A new test pins the boundaries the random masks reach only by chance:None short of, at, and one past representative multiples of 64 up to the full bitmap (FP_N_MAX= 512), with no mismatches, every base a mismatch, mismatches at bits 0 and 63 of each word, a mismatch atN - 1(an empty final run), and a score that reaches 0 with mismatches left in the same word, or on a word's last mismatch with mismatches left only in a later word. It runs five scoring schemes (includingb = 0andh0 = 1) over each layout and asserts that the empty-final-run case and both stop-with-mismatches-left cases were actually reached.N - 1is skipped, fails the new test, and recordingN - 1instead ofNfor the final run fails both the new test and the existing ones.src/is a single comment inungapped_ext.h, with its line count and itsxassertline unchanged. Every object that includesungapped_ext.h(bwamem.oand the seven unit-test objects) disassembles identically before and after it on arm64 and on an x86-64 build at the AVX2 tier.test/run_unit_tests.shpass natively on arm64 (bwtSeed lockstep parity 95/95, SMEM lockstep parity 56/56), as doband_cert_identity.shand the shell lint. A build with-DBWA_MEM3_DEBUG_UNGAPPED_XCHECKpasses the unit suite (618/618) and its end-to-end runs below trip no cross-check. On an x86-64 build at the AVX2 tier, the unit suite (597 passed, one AVX-512-only case skipped) and the bwtSeed and SMEM lockstep parity tests (95/95, 56/56) pass.main(excluding@PG) on arm64, on the arm64 cross-check build, and on an x86-64 build at the AVX2 tier: 100k pairs of the WGS slice below against hg38, SE and PE with default options, PE with-d 70,-w 30,--compat=bwa-mem2,-k 10,-k 30,-y 1,-L 0and--ks-dedup=off, and long reads (300 bp, 1 kbp, 3 kbp).mainon every timing run on all three hosts.Timing
Whole aligner,
mem -t 16on a 5M-pair WGS slice (HG00096, 2x150, hg38), clang 19.1.7, index in shared memory, 6 interleaved reps per build in a seeded shuffled order on the same host. This change was measured on its own againstmain. Figures are the paired per-rep delta vsmain(median, bootstrap 95% CI).Start-up included: the first 200k pairs of the same slice,
-t 16, with the index loaded from the page cache on every run, 12 interleaved reps:The walk runs on every architecture, so Graviton4 is a measurement here, not a null control. At 5M pairs only Sapphire Rapids excludes zero on both wall and CPU, at about −0.15% and −0.10%. On Zen 3 both intervals cross zero. On Graviton4 the wall interval crosses zero and the CPU interval just excludes it (upper end −0.01%). Every 5M median is negative. At 200k the Zen 3 and Graviton4 CPU intervals exclude zero; the Sapphire Rapids and Graviton4 wall intervals touch zero, the Zen 3 wall median is exactly 0 with its interval touching it, and the Sapphire Rapids CPU interval crosses it. Read the 200k figures with care. A 200k run takes a few seconds, so the timer's 10 ms resolution is 0.3–0.6% of a run and the wall medians sit on that grid. On Sapphire Rapids that batch also ran faster than
mainfor most changes measured alongside this one, so its 200k figure is not this change's own effect; the 5M-pair runs are the figure to go by. No interval excludes zero on the slow side.Stack
This is the thirteenth PR of a stacked series that reworks SMEM seeding, SA lookup and extension batching. It is based on #696 and should be reviewed after it. The next PR in the series is "perf(ext): probe non-HIT extensions up to 512 bases in the certified rung".
Split out of #656, thanks to @jmschrei for the original work.
Summary by CodeRabbit