CPUFJ fastpath for binary instances + latency changes to find better solutions earlier - #1750
Open
aliceb-nv wants to merge 22 commits into
Open
CPUFJ fastpath for binary instances + latency changes to find better solutions earlier#1750aliceb-nv wants to merge 22 commits into
aliceb-nv wants to merge 22 commits into
Conversation
Instances whose variables are all binary and whose rows carry integer coefficients within int8 or int16 range run an integer engine instead of the general path. Feasibility is an exact compare against one row bound after a one-sided split, so there is no tolerance arithmetic and no compensated summation anywhere; a live per-variable score is patched through stored per-nnz contributions, and move selection is a global argmax over that score while the objective weight is zero. The hot kernels are vectorized with Google Highway, compiled once per SIMD target and dispatched at runtime. They live in a host-compiled .cpp because nvcc's frontend rejects Highway's x86 headers, so the seam passes only plain pointers and scalars. Choices settled by measurement rather than argument, on an EPYC 9554 (Zen 4, AVX-512) unless noted, with supportcase22 and bnatt400 at 16 climbers: - The row remainder is masked into the vector body on targets with mask registers and peeled into a scalar tail elsewhere. Masking cost 6.8% and 12.9% on AVX2, where the mask becomes a vector and scatter is emulated. - The score read-modify-write goes lane by lane on Zen 4 rather than through VPSCATTERDD, which is 89 uops there against ~19 on SPR-class parts: +5.8%. The assignment gather stays in hardware; doing that one by lane cost 8.2%, to store-to-load forwarding on the spilled index vector. - Rows dispatch to a 4-, 8- or native-width kernel by length, since a gather costs the same whether its lanes carry data or are masked off and rows are far shorter than a 512-bit vector. Worth +2.0% on supportcase22 and +3.9% on bnatt400. - The reverse-CSR row walk carries a software prefetch. Neutral here, kept because the walk is the one data-dependent access the hardware cannot follow. Also adds solve_CPUFJ, a benchmark-only harness that runs a portfolio of climbers on one instance, and an end-of-solve audit that recomputes the incumbent's row activities in int64 and its objective from the assignment alone, trusting nothing the incremental path maintained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sweep read two bytes of tenure per variable to answer a question that is true for at most a dozen of them: one variable flips per iteration and tenure is bounded, so only that many are ever tabu. It now reads var_score alone and the caller holds the tabu few at the invalid sentinel across the call, which is the same value the masked select wrote before. That drops the scan from six bytes per variable to four. On crypt16 it takes the argmax working set from 31 KB, which overflows a 32 KB L1, to 21 KB, which does not, and total L2 requests fall 59% (685 to 256 per iteration) — more than the byte count alone predicts, because below the threshold the var_score reads stop missing too. ArgmaxImpl goes from 31.2% of cycles to 23.0%. The tabu set is a ring of the last 16 flips indexed by iteration rather than by expiry. Indexing by expiry is cheaper — position encodes the deadline, so aging is a shift and no comparison is needed — but two flips can share a deadline and the later would silently displace the earlier; simulation over the [3,12] tenure range puts that at 35% of insertions. Indexing by iteration cannot collide, since exactly one flip happens per iteration. on_flip drops any earlier entry for the variable before inserting. Without that the ring holds a superseded deadline: flip_until keeps one deadline per variable and a reflip overwrites it, possibly with an earlier one, while the ring would keep both and block on the later. A variable can be reflipped while still tabu because the local-minimum path tests the weaker iter == last_flip + 1. The first version of this patch missed it and blocked too much, which showed up as fewer crossings at higher throughput; with the fix the crypt16 trajectory is identical to before, the same ten climbers crossing 10-12% earlier. Restores run in reverse: a variable flipped twice inside the ring appears twice, and its second save holds the sentinel written by the first. Measured at 16 climbers: crypt16 +8.4%, supportcase22 +1.1%, bnatt400 -2.9%. The gain tracks n_variables, since it is a residency effect; bnatt400's 401 variables were already resident and only pay the ring's bookkeeping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…move's base pointers Two findings from a Zen 4 profile of supportcase22 (16 climbers, 20 s): 3.537M -> 3.619M aggregate iterations/s, +2.3%. apply_move ended by clearing var_bitmap in full, which GCC emitted as a tail jump to memset: 1.50% of the run spent zeroing 6,489 bytes per iteration for a guard that only find_move_in_rows reads, on the sampling paths. That function now clears the entries it set, which costs 0.20%. This is not bit-identical. find_move_satisfied and find_move_violated can both run in one iteration, and the second used to inherit the first's dedup bits and skip variables it had already seen. The carryover was accidental -- the comment there only justifies dedup within a single call -- but it was real behaviour. Only a climber that has crossed diverges; on supportcase22 the crossings hold at 1/16 and the objective moves 117 -> 116. The loop in apply_move also reloaded eleven .data() pointers out of `this` per row visit, the top three at ~1.4% each. That is aliasing, not register pressure: the body writes h.lhs through fj_bin_row_t* and nnz_score_delta[..] through int32_t*, either of which may alias a vector's internal pointer. Hoisting them into const locals removes every this-relative load from the loop and lets GCC walk reverse_constraints and reverse_coefficients by pointer induction. +1.0%, trajectory bit-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
supportcase22 goes from 3.598M to 4.348M aggregate iterations/s, +20.6%, with the search trajectory bit-identical. 85% of row visits leave their row deeply satisfied on both sides of the flip and do nothing but advance its slack. That part is uniform and now runs in Highway: fj_bin_walk_rows advances a tile of a variable's incidences and returns the ones that need scalar attention, which the caller finishes. The layout is what makes it cheap. Rows now store sign * (bound - lhs) rather than lhs, in a standalone row_slack, so the update is new_slack = old_slack - signed_coefficient[i] * delta and neither bound nor lhs appears. signed_coefficient and incident_row_cmax are replicated per incidence, so the kernel reads them at unit stride and the only irregular access left is row_slack itself: one gather and one scatter per vector, against four gathers and a scatter for a literal SoA split of the row record. That also empties fj_bin_row_t down to weight and sign. Only deep_sat is tested in the kernel. deep_viol stays with the caller because it fires on 0.02% of visits but guards the widest rows in the matrix -- deleting it outright cost 1.0% on supportcase22, since those rows average ~2,100 nonzeros and it was suppressing 16% of all patch work. The caller tiles rather than interleaving its tail into the vector loop: the lane loop sits behind a dispatch pointer that cannot be inlined, while the tail needs engine state the kernels TU cannot reach, so tiling is what keeps the patch calls outside it without a per-group callback. Trajectory verified by diffing the per-climber (iteration, viol, best, maxw) traces: identical multisets across all 16 climbers to 3,000,000 iterations each, 48,016 log points. A scalar arm for short incidence ranges was tried and dropped. Sweeping the degree below which apply_move walked the rows itself, bnatt400 degraded monotonically 14.43M -> 14.19M as the threshold went 0 -> 64, and crypt16 and supportcase22 were flat. bnatt400 (-1.1%) and crypt16 (-3.1%) do regress, but not for that reason, and the cause is still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The binary fast path's kernels are built with Google Highway, fetched at configure time like PSLP and PaPILO, but the dependency was never recorded in thirdparty/THIRD_PARTY_LICENSES. Highway is dual-licensed under Apache 2.0 or BSD 3-Clause; cuOpt elects Apache 2.0, so the entry names that licence and reproduces its text verbatim from the upstream LICENSE file. Highway ships no NOTICE file, so Apache 2.0 section 4(d) has nothing to propagate, and the sources are consumed unmodified, so 4(b) does not apply either. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
The width crossovers are per-target constants, but reaching them from the call seam turned them into loads of runtime globals: the stub compared the row length against fj_bin_n4_max and fj_bin_n8_max, selected a table entry with a cmov, loaded the pointer and jumped through it. That put an unpredictable branch on row length directly in front of an indirect jump, in a serialized cmp -> select -> load -> jmp chain, with two extra callee-saved registers spilled around it. Decide the width inside HWY_NAMESPACE instead, where the crossovers are constexpr and the three kernels are direct calls, and export one entry point per coefficient width. On a scalable target both bounds are 0, so the compares fold away and the narrow arms are stripped, which is what the runtime HWY_HAVE_SCALABLE check did before. The seam collapses from 28 instructions to one: jmp *fj_bin_patch_i8(%rip) supportcase22, 20s x 16 climbers, mean of 3: 4.243 -> 4.354 M iters/s (+2.6%), which recovers the 2.4% this seam had cost and lands slightly ahead of the shape that preceded it. Search behaviour is unchanged (1/16 crossed, objective 116). Signed-off-by: Alice Boucher <yboucher@nvidia.com>
The one-sided split emitted the lower-bound side as a'x >= lb and carried an int8 sign per row to reconstruct the slack as sign * (bound - lhs). Negating that side's coefficients and bound at build time makes every row a'x <= b, so the slack is bound - lhs everywhere and the sign disappears from the record, the kernels and every rebuild path. This costs nothing to store: emit() already pushes a separate coefficient copy per side. Negation is safe on both fields -- the eligibility scan admits |coef| up to 127 for int8 and 32767 for int16, and the bound is range-checked after negating rather than before. With sign == 1 everywhere, signed_coefficient became a plain transpose-order copy of the coefficients, i.e. identical to reverse_coefficients, which was written but never read. The two merge under the latter name, which is what the array now is. Removed: pb.sign (0.26 MB), one of the two per-nnz coefficient copies (2.2 MB), the sign field of fj_bin_row_t (leaving a bare weight), one argument from fj_bin_patch_row, and the vsign multiply from both the vector and scalar patch arms. supportcase22, 20s x 16 climbers, mean of 3: 4.354 -> 4.396 M iters/s (+0.96%). Trajectory is bit-identical: 84951 common (climber, iteration) samples across all 16 climbers match on viol, best and maxw, and the incumbent audit passes on every run. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
Once the sign was normalized away, fj_bin_row_t was a single int32 field in a struct still templated on a coef_t it never used. It is now a std::vector<int32_t> row_weight on the engine, which drops the type, its template parameter, and the reference-into-the-array idiom at every use site. Storage is unchanged -- four bytes per row either way -- and the trajectory is bit-identical: 88030 common (climber, iteration) samples across all 16 climbers match on viol, best and maxw. This costs 0.57% on supportcase22: 4.400 -> 4.375 M iters/s, both means of four runs measured back to back to rule out machine drift. The cause is not in the work done. apply_move comes out smaller and no busier -- 822 instructions against 832, 37 spill stores against 38, identical reload and branch counts -- and the diff is a register allocation and stack slot reshuffle across the whole function rather than anything localized. Same class of second-order codegen effect as the ArgmaxImpl relocation that cost 123 cycles per iteration earlier with a byte-identical instruction stream. Taken deliberately: the simpler type is worth more than 0.5% here. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
The tile width of the argmax sweep was ported from the prototype as a bare constant, dropping the residency guard that computed it. The target of 256 is about the shape of the sweep -- it sets how often the running maximum is raised, which is what bounds the index re-scan -- but the re-scan only pays off because it revisits a tile that is still L1-hot. On a small L1 an unguarded 256 pushes that re-read out to L2 and costs more than the split saves, which is what the cap the prototype applied was for. Bytes per variable is the score array alone. The prototype also counted its u16 flip_until, because its sweep tested tabu per lane; this engine blocks the handful of tabu variables at the invalid sentinel before the sweep and restores after, so flip_until is never touched here and the divisor is 4 rather than 6. No behaviour change on the machines we run: at 32 KiB the cap is 2048 and the target binds. supportcase22, 20s x 16 climbers, mean of 3: 4.375 -> 4.364 M iters/s, within the layout noise seen across today's commits, with 1/16 crossed as before. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
The binary fast path and the general path are meant to search identically, so a divergence between them is a bug in the fast path. Setting this env var makes try_cpufj_binary_solve decline unconditionally, which is what turns that comparison into one command on an instance the fast path would otherwise take. Found its first bug already: on chromaticindex1024-7 the general path crosses 16/16 in ~30s with 65k iterations per climber, while the fast path runs 1M iterations per climber and crosses 0/16, never reaching a local minimum and so never engaging DDFW. The cause is packed-score saturation -- the engine's own report gives aggregate bonus 122880 against a limit of 16384 -- which corrupts the lexicographic order the packing encodes. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
The packed score encodes the general path's lexicographic (base, bonus) comparison as one arithmetic comparison, and at 15 bits the bonus field was too narrow to do it. Both fields aggregate over the rows a variable appears in, so each is bounded by max_var_degree * max_weight -- unbounded at build time, since DDFW grows the weights. Where the bonus exceeded 2^14 it carried into the base, manufacturing improvement that was not there, so the argmax ranked moves by a corrupted key, no local minimum was ever detected, and DDFW never engaged. Measured aggregate bonus against the old 16384 limit: chromaticindex1024-7 122880 (7.5x), 30n20b8 binarized 66396 (4.1x), crypt16 20229 (1.23x), bnatt400_reduced 17611 (1.07x). Only supportcase22 stayed inside, at 5751. Shift becomes 32, so base has the whole int32 range before the encoding can break. The two fields are per-row where they are computed, so the kernel still evaluates them as int32 at full lane count and widens only for the pack; the pack, the previous value, the difference and the store back all stay in the vector, leaving the scalar lane loop at one add per nonzero as it was before. That loop is 38% of all cycles, so it is the wrong place for work: a first version that packed there instead cost 26.7% rather than 17.8%, and a variant that also encoded the per-nnz delta as two int16 fields to halve that array cost 31.5% -- the patch path is instruction-bound, not bandwidth-bound. chromaticindex1024-7 goes from 0/16 crossings to 16/16, first crossing at 0.13s against 23-33s on the general path, same objective 4. supportcase22 costs 17.8% (4.364 -> 3.589 M iters/s) and is the one instance whose trajectory is unchanged, which is what the saturation numbers predict. crypt16 11/16 and bnatt400_reduced 16/16 objective 1 both hold. Not explained: on supportcase22 two of sixteen climbers diverge from the int32 trajectory past iteration 4.25M (887 of 72338 sampled points); the other fourteen are bit-identical. Left open deliberately rather than dismissed. Signed-off-by: Alice Boucher <yboucher@nvidia.com>
aliceb-nv
added
non-breaking
Contributor
Author
|
/ok to test de1a8bd |
CI Test Summary⏭️ All 5 test job(s) skipped. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR adds a new SIMD-tuned fastpath for CPUFJ for the case of binary-only instances with certain restrictions. It also incorporates multiple changes to help find feasible solutions earlier in the solve.
Benchmark results pending. Initial results show the SGM primal integral going down from 0.030 to 0.026.
Description
Issue
Checklist