Repository navigation
perf(fmi): keep the backward step's loop invariants in registers - #677
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 |
|---|---|
Cached backward extension inputs src/FMI_search.h, src/FMI_search.cpp |
backwardExt_konly delegates to a helper that accepts the checkpoint pointer and base count. The general loops cache index and checkpoint values, use the helper, and write updated loop state back. The single-entry specialization comment now notes its parity requirement and test coverage. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related PRs
- fg-labs/bwa-mem3#676: Updates checkpoint prefetches in the same lockstep drivers.
- fg-labs/bwa-mem3#672: Changes earlier SMEM and bwtSeed walks in the same optimization series.
- fg-labs/bwa-mem3#674: Adds a first-walk hint path to the same lockstep drivers.
Merge Risk: 
09b67
-
The optimization preserves the inspected backward-search calculations and state updates. No actionable merge-blocking issue remains; merge after normal checks pass.
Important
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Public-Repo Hygiene |  |
The visible title and diff contain no prohibited internal tool, bucket, account, hostname, IP, or local path. The supplied PR body is truncated after “on Grav…”. The full body is required for this che… | Provide the complete PR body, or rerun the check with the full description available. Then scan the omitted text for non-public benchmark names, object-store references, account or instance IDs, internal hostnames, private IPs, and local fi… |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title follows Conventional Commits with the perftype andfmiscope. It names the touched FMI component and accurately describes the loop-invariant performance change.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 The PR changes only FMI_search.cpp and FMI_search.h. The helper preserves the existing arithmetic while taking hoisted cp_occ and count[a] values, and the general loops preserve the prior predicates, … Kernel Change Validation Reported PASS: The pull request changes only src/FMI_search.cppandsrc/FMI_search.h. It does not modify any file covered by this check, so the description-reporting requirements do not apply.Simd Tier Coverage No SIMD tier coverage risk is introduced. The diff changes scalar loop state and moves existing backward-extension arithmetic into backwardExt_konly_cp; it adds no vector intrinsic, tier dispatch, `…Full details: Equivalence Contract Updated
-
Explanation
The PR changes only FMI_search.cpp and FMI_search.h. The helper preserves the existing arithmetic while taking hoisted cp_occ and count[a] values, and the general loops preserve the prior predicates, write order, and state updates. The PR description explicitly states that output is byte-identical and records verification through unit, lockstep-parity, cross-tier, and end-to-end comparisons on arm64 and x86 AVX2. This satisfies the contract even if the SMEM path is treated as potentially output-affecting; no equivalence-document update is required when the description gives explicit unchanged-output verification.
Full details: Simd Tier Coverage
-
Explanation
No SIMD tier coverage risk is introduced. The diff changes scalar loop state and moves existing backward-extension arithmetic into
backwardExt_konly_cp; it adds no vector intrinsic, tier dispatch,SIMD_WIDTHuse, or SIMD-dependent buffer size. The four changed_mm_prefetchcalls only replacecp_occwith the cachedcpopointer. Existing target-specificocc_popcount64SIMD code is unchanged.
-
Full details: Public-Repo Hygiene
-
Explanation
The visible title and diff contain no prohibited internal tool, bucket, account, hostname, IP, or local path. The supplied PR body is truncated after “on Grav…”. The full body is required for this check, and no full PR-description source is available in the repository or review refs.
Resolution
Provide the complete PR body, or rerun the check with the full description available. Then scan the omitted text for non-public benchmark names, object-store references, account or instance IDs, internal hostnames, private IPs, and local filesystem 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.
✅ Action performed
|
|
@coderabbitai pause |
✅ Action performed
|
ead8e02 to
4b15591
Compare
e2d25ce to
f9343d2
Compare
4b15591 to
25a09be
Compare
f9343d2 to
a31e381
Compare
25a09be to
4897b39
Compare
ls_advance_backward_step's two general loops read the slot's prev pointer, numPrev, min_intv and j, and the index's cp_occ and count[a], through s and this on every iteration. The SMEM stores into prev[] may alias those fields as far as the compiler can tell, so it reloads them after each store, one extra L1 load ahead of each prev[p] load, compare and prefetch. Read them once into locals before the loops and write numPrev and j back once after. backwardExt_konly_cp is backwardExt_konly on a caller-held cp_occ pointer and count[a] (backwardExt_konly now forwards to it), so the loops can pass the hoisted values. Its comment states that it keeps backwardExt_konly's contract: k is left stale on the s'->0 exit, so the caller must discard any s' < min_intv. prev[] and match_buf[] are driver-owned buffers that overlap neither the slot nor the index, so the result is byte-identical. The numPrev == 1 path is unchanged. Co-authored-by: Jacob Schreiber <jacob.schreiber@umassmed.edu>
a31e381 to
09b679f
Compare
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action performed
|
Summary
The lockstep SMEM driver's backward step (
ls_advance_backward_step) extends every interval in a slot'sprev[]by one base per outer step. Its two general loops (taken whenevernumPrev > 1) read the slot'sprevpointer,numPrev,min_intvandj, and the index'scp_occandcount[a], throughsandthison every iteration. The loops storeSMEMs back intoprev[], and as far as the compiler can tell those stores may alias the slot and index fields, so it reloads them after each store: one extra L1 load ahead of eachprev[p]load, compare and prefetch. This PR reads them once into locals before the loops and writesnumPrevandjback once after. Output is byte-identical.prev,numPrev,min_intv,j,cp_occandcount[a]become locals for the two general loops;s->numPrevands->jare written once after them (s->j = j - 1, the olds->j--).backwardExt_konly_cpisbackwardExt_konlyon a caller-heldcp_occpointer andcount[a], so the loops can pass the hoisted values.backwardExt_konlynow forwards to it, so the arithmetic lives in one place. Its comment states that it keepsbackwardExt_konly's contract:kis left stale on the s'→0 exit, so the caller must discard any s' <min_intv.numPrev == 1straight-line path, the forward phase and the bwtSeed driver.Correctness
s->numPrev,s->j,s->min_intv,s->prev,cp_occorcount. The only slot writes ares->cur_j,s->match_countand amatch_buf[]entry, each followed bybreak, andprev[numCurr++]writes at or behind the read indexp, as before.prev[]andmatch_buf[]are driver-owned buffers that overlap neither the slot nor the index, so the hoisted values equal what the reloads would have read.test/run_unit_tests.shpasses natively on arm64 (bwtSeed lockstep parity 95/95, SMEM lockstep parity 32/32; the SMEM parity test takes the general-loop path about 6,000 times). 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): 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 a gain on all three hosts; on Graviton4 the median is −0.8% but the interval reaches just past zero. 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 sixth PR of a stacked series that reworks SMEM seeding and SA lookup. It is based on #676 and should be reviewed after it. The next PR in the series writes the forward phase's
prev[]entries downward.Split out of #656, thanks to @jmschrei for the original work.
Summary by CodeRabbit