Repository navigation
perf(fmi): drop the cross-slot T1 look-ahead from the seeding drivers - #676
Conversation
|
@coderabbitai pause |
|
Note Reviews paused
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Remove cross-slot T1 prefetches src/FMI_search.h, src/FMI_search.cpp, docs/src/whats-different/performance.md |
The drivers no longer call the cross-slot T1 prefetch helpers, and the private helper declarations and implementations are removed. Same-slot prefetches remain. Comments and performance documentation describe the prefetch behavior. |
Update stack-size estimates src/FMI_search.cpp, src/lockstep_width.h |
Comments update the estimated per-slot hot-state size to 160 B and the estimated worst-case stack frame at width 256 to 43 KB. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related PRs
- fg-labs/bwa-mem3#626: Changes the lockstep SMEM driver that this PR also modifies.
#626adds same-pass slot retirement and refill; this PR removes cross-slot T1 prefetching from that driver.
Merge Risk: 
538f9
-
No actionable merge-blocking risk is identified. The change removes cross-slot prefetch hints without changing the described seeding logic and is mergeable subject to normal checks.
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Public-Repo Hygiene |  |
The reviewed diff and commit contain no object-store URL, bucket, account number, instance ID, internal hostname, or machine-local path. Added lines use the public PR URL, clang, and standard CPU/clou… | Provide the complete pull-request body and review metadata, or confirm that the omitted body text contains no non-public benchmark, tool, service, bucket, account, hostname, or local path. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the required Conventional Commits format, names the FMI component and seeding drivers, and accurately describes the removal of cross-slot T1 look-ahead prefetching. 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 executable change in src/FMI_search.cpponly removes_MM_HINT_T1prefetch helpers and their cross-slot calls. It does not modify scoring, tie resolution, MAPQ, flags, tags, CIGAR constru…Kernel Change Validation Reported The pull request does not modify src/bandedSWA.cpp, src/kswv.cpp, src/ksw.cpp, src/neon_utils.h, or src/simd_compat.h. The conditional documentation requirements do not apply. Simd Tier Coverage PASS: The diff removes cross-slot _MM_HINT_T1prefetch helpers and their driver calls. It adds no SSE, AVX, or NEON intrinsic, changes no tier dispatch, and changes noSIMD_WIDTH-dependent buffer.…Full details: Equivalence Contract Updated
-
Explanation
PASS: The executable change in
src/FMI_search.cpponly removes_MM_HINT_T1prefetch helpers and their cross-slot calls. It does not modify scoring, tie resolution, MAPQ, flags, tags, CIGAR construction, or record selection. The PR description explicitly states that output is byte-identical and reports verification againstmainacross lockstep and scalar runs. No equivalence-contract update is required.
Full details: Simd Tier Coverage
-
Explanation
PASS: The diff removes cross-slot
_MM_HINT_T1prefetch helpers and their driver calls. It adds no SSE, AVX, or NEON intrinsic, changes no tier dispatch, and changes noSIMD_WIDTH-dependent buffer. Thelockstep_width.hedits are comments only. Existing same-slot_mm_prefetchcode remains unchanged.
-
Full details: Public-Repo Hygiene
-
Explanation
The reviewed diff and commit contain no object-store URL, bucket, account number, instance ID, internal hostname, or machine-local path. Added lines use the public PR URL, clang, and standard CPU/cloud names. The pull-request body is truncated in the supplied context, and no full body is available in the checkout. Because this check applies to the complete body, the hygiene assessment cannot be complete.
Resolution
Provide the complete pull-request body and review metadata, or confirm that the omitted body text contains no non-public benchmark, tool, service, bucket, account, hostname, or local path.
Finishing Touches
-
Fix failing CI checks
-
- Commit to this branch
- Create a new PR
-
- 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.
4f03d31 to
538f979
Compare
✅ Action performed
|
|
@coderabbitai pause |
✅ Action performed
|
|
@coderabbitai review |
✅ Action performed
|
45897ef to
26d3936
Compare
538f979 to
22f40ac
Compare
26d3936 to
6d5dc22
Compare
22f40ac to
ead8e02
Compare
6d5dc22 to
18c6687
Compare
ead8e02 to
4b15591
Compare
18c6687 to
2b6fa11
Compare
4b15591 to
25a09be
Compare
2b6fa11 to
1c83384
Compare
Both lockstep seeding drivers (SMEM and bwtSeed) issued, after every slot visit, an L2 prefetch of the two checkpoint blocks that the slot N/2 places further on would read next. For a slot whose next step reads checkpoint blocks, the hint repeats one already in flight: the slot T0-prefetched those blocks at the end of its own last step, at slot init, or (for the first backward step) in ls_prepare_backward. For a slot whose next visit reads something else (a planned jump-table entry, the unique-match LF walk or text compare, or nothing once it is done) it warmed blocks no one reads. It also cost a phase test, mask selects, and in the bwtSeed driver a 32-bit division per visit. Without it the whole aligner is faster on all three SIMD tiers measured (AVX-512BW, AVX2, NEON). Remove ls_prefetch_cp_occ_t1 and bsd_prefetch_cp_occ_t1 and their calls. The drivers' comments now say which same-slot prefetch covers each slot's next visit, the SMEM driver's comment gives the hot slot's actual size (160 bytes), and the PR #584 entry in performance.md notes that the look-ahead has since been dropped. Prefetches are hints only, so output is byte-identical. Co-authored-by: Jacob Schreiber <jacob.schreiber@umassmed.edu>
25a09be to
4897b39
Compare
|
@coderabbitai review |
Action not completed
|
Summary
Both lockstep seeding drivers (SMEM and bwtSeed) issued, after every slot visit, an L2 (
_MM_HINT_T1) prefetch for the slot half a batch ahead (slots[(s + N/2) % N]): the two checkpoint blocks that slot would read on its next step. Each slot already hints what its own next step reads into L1, so the look-ahead only repeated those hints, and it paid for them on every visit. This PR removes it. Output is byte-identical.ls_prepare_backwardfor the first backward step, or when it was filled (ls_prefetch_cp_occ,bsd_prefetch_cp_occ). A slot whose next visit reads something else (a planned jump-table entry from perf(fmi): take the seeding walks' first steps from a jump table #672, the unique-match LF walk or text compare from perf(fmi): compare unique matches with the text and keep their position for SA lookup #670, or nothing once it is done) got a hint for blocks it does not read.jtest plus a 32-bit% Ndivision (bwtSeed driver), and two prefetch instructions.ls_prefetch_cp_occ_t1,bsd_prefetch_cp_occ_t1and their calls. The drivers' comments now say which same-slot prefetch covers each slot's next visit, and the SMEM driver's comment gives the hot slot's actual size (160 bytes). The "Seeding lookahead prefetch" entry under perf: drop coin-flip branches in chaining, ungapped extension and seeding #584 indocs/src/whats-different/performance.mdnow notes that the prefetch has since been dropped.Correctness
_mm_prefetch.test/run_unit_tests.shpasses natively on arm64 (bwtSeed lockstep parity 71/71, SMEM lockstep parity 32/32). On an x86-64 build at the AVX2 tier, both lockstep parity tests pass. x86 compile checks pass at-mavx2and-mavx512f -mavx512bw.main(excluding@PG) on arm64 and on an x86 AVX2 build, with the lockstep driver (the default) and with the scalar driver (BWA3_BWTSEED_LOCKSTEP=0), and on arm64 also at lockstep widths of 3 (BWA3_SMEM_LOCKSTEP_N=3,BWA3_BWTSEED_LOCKSTEP_N=3), where the old look-ahead target wrapped: SE, PE and long-read runs with default options,-d 70,-w 30and--compat=bwa-mem2, and PE with--ks-dedup=on/off,-k 10,-k 30,-y 1,-y 5andBWA3_BWTSEED_JUMP=0/12.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 5M-pair runs show the steady-state gain on every tier. 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; the CPU-time intervals there exclude zero on all three hosts.
Stack
This is the fifth PR of a stacked series that reworks SMEM seeding and SA lookup. It is based on #674 and should be reviewed after it: the comments it rewrites name the jump-table (#672) and unique-match (#670) steps whose own prefetches cover a slot's next visit. The next PR in the series keeps the backward step's loop invariants in registers.
Split out of #656, thanks to @jmschrei for the original work.
Summary by CodeRabbit
Performance
Documentation