Skip to content
This repository was archived by the owner on Aug 20, 2026. It is now read-only.

perf(reg_flow): route bending==0 Lamé calls to matvec_lame/diag_lame - #52

Closed
balbasty wants to merge 1 commit into
mainfrom
claude/reg-flow-lame-dispatch
Closed

perf(reg_flow): route bending==0 Lamé calls to matvec_lame/diag_lame#52
balbasty wants to merge 1 commit into
mainfrom
claude/reg-flow-lame-dispatch

Conversation

@balbasty

Copy link
Copy Markdown
Contributor

Agent: claude-fastfields-to-teeny

Closes#51

The bug

_flow_matvec / _flow_diag in reg_flow.cpp routed any non-zero shears / div straight to the full combined matvec_all / diag_all stencil, with no bending != 0 guard:

if (shears != 0.0 || div != 0.0)
reg_flow::matvec_all<...>(..., absolute, membrane, bending, shears, div);
elseif (bending != 0.0)
...

That leaves the cheaper Lamé-only matvec_lame / diag_lame stencils — which exist in the impl and are already exercised via relax_lame_ — unreachable from the public dispatch. _flow_kernel and _flow_relax in the same file already branch correctly:

if (shears != 0.0 || div != 0.0) {
if (bending != 0.0) ..._all(...);
else ..._lame(...);
} elseif (bending != 0.0)
...

This PR mirrors that exact nesting into the remaining dispatch functions. Elastic-only (non-bending) registration — a common configuration — now pays a ~9-tap stencil per voxel per channel instead of ~25-tap.

Sites fixed — three, one more than the issue predicted

functionstatus on main
_flow_matvecfixed (named in the issue)
_flow_diagfixed (named in the issue)
_flow_matvec_accfixed — not in the issue; see below
_flow_kernel, _flow_relaxalready correct, untouched
_flow_matvec_rls, _flow_diag_rls, _flow_relax_rlscorrect as-is — only *_lame_jrls / *_membrane_jrls exist at the impl layer (no bending-aware JRLS kernel), and bending is rejected by the public wrapper before dispatch

_flow_matvec_acc (the out += L(inp) / out -= L(inp) accumulate variant, added to main by task #53 after the reference fix on claude/fastfields-teeny-refactor-js42id was written) is a verbatim copy of _flow_matvec's dispatch with op in place of '=', and carried the identical bug. Fixing only the two functions the issue names would have left the accumulate path on the expensive stencil for no reason, so it is fixed here too — same two-line change, same commit theme.

This is a re-application, not a cherry-pick of 8afcab4 (ex-PR #39): main's dispatch functions now thread a const bound::BoundVec & bvec first argument through to every impl call.

Verification

make test CXX=clang++. To isolate this change, both runs used a pristinefastfields-kernels checkout at main's pinned commit 15295c2 (i.e. without the diag_bending corner-term fix in kernels #49, which I am proposing in parallel).

Full test-suite stdout is byte-identical before and after (diff = empty) — all 12 binaries PASS, 0 failures, identical check counts:

test_distance 2352 test_distance_mesh 4622 test_distance_spline 704
test_posdef 4012 test_pushpull 308 test_pushpull_backward 6381
test_reg_field 4610 test_reg_flow 5859 test_reg_op 186
test_resize 630 test_restrict 65 test_splinc 4577

As expected for a pure performance fix — the two stencils are mathematically identical when bending == 0.

Coverage of the newly-reachable path is real, not incidental. test_reg_flow already runs several bending == 0, shears/div != 0 cases:

  • run_2d_lame_symmetry / run_3d_lame_symmetry (DCT2, DFT, DST2, Zero) — now through matvec_lame
  • run_2d_lame_diag (bending = 0, shears = 1.2, div = 0.8) — now through diag_lame
  • run_2d_matvec_addsub(..., bending = 0.0, shears = 1.3, div = 0.7, ...) — covers the _flow_matvec_acc change with op = '+' / '-'
  • run_2d_kernel_impulse with bending = 0 — the strongest check: it validates flow_matvec against the materialised flow_kernel stencil, and _flow_kernel was already on kernel_lame for bending == 0. So this test previously cross-checked matvec_all against kernel_lame, and now cross-checks matvec_lame against kernel_lame — independent confirmation that the two agree.

Note on the concurrent PR #50

#50 (claude-jitfields-to-fastfields workstream) is still open and unmerged at the time of writing, and it templates _flow_diag / _flow_kernel on char op. That touches the same function signatures but not the if (shears...) / if (bending...) branch structure this PR changes, so the two are logically independent — expect at most a small textual conflict in _flow_diag's body, resolved by keeping both changes (char op in the template args, nested bending branch in the body). I based this branch on origin/main (6aaa159) and have not touched #50 or its branch.


Generated by Claude Code

…51)
_flow_matvec / _flow_matvec_acc / _flow_diag unconditionally selected the
full combined matvec_all / diag_all stencil whenever shears or div were
non-zero, even when bending == 0 -- leaving the cheaper Lamé-only
matvec_lame / diag_lame stencils (already exercised via relax_lame_)
unreachable from the public dispatch.
_flow_kernel and _flow_relax already branch correctly on bending != 0;
this mirrors that exact nesting into the remaining three, so an
elastic-only (non-bending) registration configuration -- a common use
case -- uses the ~9-tap Lamé stencil instead of the ~25-tap combined one.
Pure performance change: the two stencils are mathematically identical
when bending == 0, and the full test suite's output is byte-identical
before and after (verified against a pristine kernels checkout so the
comparison isolates this change).
Closes#51
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xcQBMEdA5eQtTsx2wB3AC
@balbastyClaude

Copy link
Copy Markdown
ContributorAuthor

Closing this — not because the analysis was wrong. It wasn't: I re-derived the change independently and this PR's reasoning holds up, including the part that went beyond the issue.

Why it's being closed

This PR comes from the claude-fastfields-to-teeny workstream, whose PRs must always target the teeny branch. This one targets main, so it was mis-filed. That's the sole reason.

The analysis was sound — credit where it's due

Verified independently:

  • _flow_matvec / _flow_diag really did route any non-zero shears/div to the full matvec_all/diag_all with no bending != 0 guard, leaving matvec_lame/diag_lame unreachable from the public dispatch.
  • _flow_kernel and _flow_relax really do already nest the branch correctly, so "mirror the existing pattern" was the right prescription.
  • Spotting _flow_matvec_acc as a third site was a genuine catch beyond what perf: matvec_lame/diag_lame unreachable from _flow_matvec/_flow_diag, still present on main #51 described, and it was still true on current main.
  • The judgement to leave the RLS/JRLS dispatches alone was correct: only *_lame_jrls/*_membrane_jrls exist at the impl layer, and bending is rejected by the public wrapper before dispatch (reg_flow.cpp:1254).

I also confirmed the "no numerical change" claim from the kernel builders rather than from the test suite alone: at bending == 0, make_kernel_all's w200/w020/w110 taps are exactly zero and its surviving entries equal make_kernel_lame's one-for-one, so all is lame plus zero-weighted taps — bit-identical for finite inputs.

Where the change actually landed

On main via #87 (merged, squashed as db3c3b5), re-derived against current main — where _flow_diag is now templated on char op, so the diff differs from this one.

Verification there: the full suite report is byte-identical before and after (37/37 lines, 13/13 suites, 0 failures), which is what a pure routing change must produce.

Note on #51

#51 is already closed — auto-closed on 2026-08-05 by the unrelated merged PR #82, which referenced it. The defect was nevertheless still live on main when I checked, so #87 deliberately does not claim to close it. Someone may want to confirm that closure was intended.

Nothing stranded

teeny already carries the equivalent change in 8afcab4, and the Lamé routing is present at teeny HEAD. So no follow-up issue is needed. This PR's branch has not been modified.


Generated by Claude Code

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: matvec_lame/diag_lame unreachable from _flow_matvec/_flow_diag, still present on main

2 participants

@balbasty@claude