test(pixelflow-codegen): close x86_64.rs mutation gaps - #1054
Conversation
Adds byte-exact unit tests for the x86-64 SSE/VEX instruction emitters (movaps load/store, f32 constant embedding, the general VEX encoder, context-pointer load, vpextrd/vmovss gather helpers, rsp-relative spill store/load, ternary MulAdd, epilogue, movmskps, cmp eax) that previously had zero direct coverage beyond end-to-end float-value comparisons. Removes emit_xorps/emit_andps: dead code with zero callers anywhere in the repo (Neg/Abs use the VEX emit_vxorps/emit_vandps instead). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
The offset=128 boundary tests for emit_movaps_load/store used Reg(0), so the ModRM reg-field shift (<< vs >>) was a no-op either way and the mutant survived. Switch to a nonzero register to distinguish them. Also cargo fmt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
…aps) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
🤖 Hi @jppittman, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
… against upstream Vex-builder refactor Upstream (#1055-#1062) replaced the raw byte-pushing SSE/VEX helpers with a Vex/Digit builder abstraction and merged regalloc.rs's driver logic into this file. Took main's version of x86_64.rs wholesale; the byte-exact tests added in this branch targeted functions that no longer exist and need to be rewritten against the current API in a follow-up commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e077cb8118
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
… Vex-builder refactor Rewrites the mutation-gap-closing tests against upstream's new Vex/Digit builder API (main moved to it while this branch was in flight, via #1055-#1062). Kept the scope to this file's real API boundary and to genuine logic bugs rather than pinning private-method delegation: - Vex::head's W-bit shift (real bug: `<<` vs `>>` only diverge when w=1, which no current caller passes, so the encoder is tested directly rather than through a production call site). - emit_movups_store_base's REX.R/B computation (four register high/low combinations). - emit_load_ptr_from_ctx, emit_movmskps_eax, emit_cmp_eax_imm8 — this file's byte-in/byte-out contract. - x86_redzone_disp's negation and disp8 overflow boundary. - emit_binary_safe's aliasing decision (the one case that must route away from the plain two-operand `emit_binary` call). - X86Backend::prologue/epilogue's frame_bytes>0 boundary: a real gap invisible to any execution/value test, since the wrong branch only emits a harmless dead `sub/add rsp, 0` pair, not a wrong answer. Most of the remaining `cargo mutants` misses in this file are `| to ^` inside ModRM/VEX/REX byte construction — provably equivalent, since those operands are always bit-disjoint fields by construction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:4435869ff6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
…factor Documents the full arc: the first draft's tests targeted x86_64.rs before upstream's #1055-#1062 rewrote it around a Vex-builder abstraction, the merge that discarded the stale draft, and the finding that a whole class of apparent driver/scaffold gaps was actually already covered by tests/collapse_loop.rs -- invisible only because this series' mutants filter has always scoped to `--lib`. Adds a methodology note recommending future passes check for a relevant integration-test target before writing new unit tests against a mutants report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui
Records the AVX2/AVX-512 backend mutation-testing pass: methodology notes on cargo-mutants' cfg-blindness (a "missed" mutant on this file can be dead code under the active RUSTFLAGS rather than a real gap), the confirmed-zero real-gap final state for both tiers, and an updated backlog noting the four unmerged draft PRs (#1049-#1051, #1054) from prior passes that this session did not touch.
Records the AVX2/AVX-512 backend mutation-testing pass: methodology notes on cargo-mutants' cfg-blindness (a "missed" mutant on this file can be dead code under the active RUSTFLAGS rather than a real gap), the confirmed-zero real-gap final state for both tiers, and an updated backlog noting the four unmerged draft PRs (#1049-#1051, #1054) from prior passes that this session did not touch.
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…-ABI refactor main's #1082 ("one kernel ABI, one compile entry") landed while this branch was up to date, deleting IsaBackend::prologue/epilogue (and the `if self.frame_bytes > 0` conditional they gated) in favor of frame_alloc/frame_free, and folding emit_movups_store_base into emit_movups_store<D: Disp>. The merge from main therefore left this branch's own tests referencing functions that no longer exist. - emit_movups_store_base_* tests now drive emit_movups_store through a NoDisp address, which is the same encoding the deleted function produced. - x86_redzone_disp's overflow-boundary test compares against CompileError::Internal(..) instead of a bare &str, matching #1081's CompileError migration. - The four prologue_*/epilogue_* tests are removed rather than rewritten: the frame_bytes>0 conditional they exercised was deleted outright by #1082, not renamed or moved. frame_alloc/frame_free are unconditional delegations to emit_sub_rsp/emit_add_rsp now, called by the shared collapse-loop scaffold with a total that is never zero, so there is no surviving branch for these tests to target. cargo test -p pixelflow-codegen (all targets incl. collapse_loop.rs): pass. cargo clippy -p pixelflow-codegen --all-targets: clean. cargo fmt -p pixelflow-codegen -- --check: clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
… segregate / delete inventory (#1090) ## Summary One docs file, `docs/plans/2026-09-01-dead-code-with-ideas.md`, answering JP's 2026-09-01 prompt ("I think we still have a lot of dead code with some decent ideas in it. factored.rs and so on"). Read-only audit of `origin/main` @ `83015dcd` plus the open branches for #1044, #1083, #1084, #1085, #1072, #1086. **No source changes** — JP decides deletions. Every row cites `file:line` and the caller grep that produced its status. Verdicts follow the segregate-don't-delete doctrine plus one new verdict for this pass, REUSE-R2 (a dormant piece whose idea the Guide's candidate-context design needs, with the plug-in point named). ## Verdict counts | verdict | items | LOC | |---|---|---| | KEEP (live) | 18 | ~16,900 (all live callers verified) | | REUSE-R2 | 6 | ~3,900 file / ~2,150 non-test — `GraphAccumulator` binding primitive, `unified_backward` adjoint, `episodes.rs` budget sampler, `init_with_latency_prior`, `bilinear_score`/`encode_rule_from_arena`, `pixelflow-ml` SH/RFF family (idea only) | | SEGREGATE | 5 | ~2,180 (4 stale docs → archive; whole-graph scoring tower already segregated; #994 is JP's) | | DELETE | 6 | **~1,357 Rust LOC on main** + 367 lines of stale doc + close#1044 (2,690 LOC that never lands) | ## Things the scans did not expect - `CostModel::load_or_default` (`cost.rs:410`) has **zero callers** and probes `$HOME/.config/pixelflow/cost_model.toml` and a nonexistent `pixelflow-ml/data/`; its only writer is `pixelflow-core/src/bin/calibrate_costs.rs` (Jun 11, superseded by `measure_latency_prior.rs`). ≈700 LOC incl. the tests #1051 merged today — decision D3. - The four `pixelflow-pipeline/examples/{op_cost_stress,egraph_choices,ilp_benchmark,critical_path_test}.rs` touch no pixelflow crate at all (scalar `f32` loops) and carry `cargo run -p pixelflow-ml` headers — 636 LOC, DELETE. - The 2026-09-01 candidate-context design branch (`claude/phase3-context`) is not on origin; the on-main design of record is `2026-08-31-guide-design-revision.md` §4, which already argues candidate-local over whole-graph. #1084 built `CandidateFeatures` without reusing `GraphAccumulator` — the reuse is the `shift_by`/Hadamard binding primitive at candidate scope, not the accumulator struct (seam R1). ## Open-PR disposition (§5) #1044 close + cherry-pick its 2 journal lines; #1049 close/fold; #1051 merged; #1054 merge after #1086's one caveat; #1050 already closed (moot as predicted); #994 JP's. Consistent with #1086's landing order (#1053 → #1085 → #1084/#1083). ## Test plan Docs only. No `cargo` gates affected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: JP Pittman <jppittman@jpptech.dev> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…cted (#1089) ## Summary Second pass over the open PRs, continuing #1086. That PR did the rebase sweep and wrote recommendations; this one records what was **executed** against them and re-derives the board rows that have since gone stale. Adds one document: `docs/results/2026-09-01-open-pr-sweep-followup.md`. ## What was executed | Action | PR | Basis | |---|---|---| | Merged | #1051 — `cost.rs` mutation gaps | test + docs only, 0 unresolved threads, all blocking checks green | | Merged | #1049 — `graph.rs` test renames | test + docs only, 0 unresolved threads, all blocking checks green | | Closed | #1050 — `regalloc.rs` mutation gaps | target code verified absent from `main` and from its own branch | | Reviewed | #1044 | `shepherd` label; found it had never been reviewed | | Retitled | #1087 | `CL metadata` wanted a conventional-commit prefix | After merging, all 11 remaining open PRs were re-checked with `git merge-tree --write-tree` against the new `main` — **all still merge cleanly**, so the merges introduced no conflicts. ## Corrections to #1086's board #1086 was accurate when written and is now one step behind. The one that moved against the trend: **#1054 went from green to red.** #1082 removed `X86Backend::prologue`/`::epilogue` and #1081 changed the error type, so its tests no longer compile (`E0599` ×4, `E0308`); Clippy, both test jobs and the ISA matrix are red. This is the *second* encoder refactor to invalidate the branch — the first is already in its own history as `4435869f`. Recommendation is to hold it until that file stops moving rather than fund a third re-close pass. ## Three things #1086 could not have seen - **The saturation collision has five participants, not three.** #1044's `variants.rs` calls `eg.saturate_with_limit(64)` (lines 229, 262), which #1085 deletes. Disjoint files, so git merges clean and the *build* breaks on whichever lands second. - **#1083 and #1087 are two mechanisms for one fact** — a real `SaturationStopReason` field vs. outside-in inference — on confusingly adjacent branch names. #1083 went green during the sweep. One should be picked before either lands. - **#1044's zero unresolved threads is an artifact.** Codex hit its usage limit on 2026-08-28 before reviewing it, so thread count ranks an unreviewed 3060-line diff as the cleanest thing in the set. ## Verified rather than relayed #1072's call-overhead P1 is real: `bench_extraction_3way.rs:2607` aggregates `bench.ns * normalization` while `adjusted_ns` is only serialized at `:2591`. Worth noting the direction — adding a constant to both arms pulls the ratio toward 1, so the true regression is *larger* than the reported 1.0153. The qualitative verdict survives; the intervals do not. ## What this pass could not do The remaining gap to "no unresolved comments, no CI failures" is entirely commits belonging on other branches, which this session is not scoped to push. The four outstanding fixes (#1083 `saturation-telemetry = ["std"]`; #1053 stale script header; #1044 `saturate_with_limits`; #1087 `cargo fmt`) are written down in the doc with exact locations. Roughly 60 unresolved threads remain across 8 PRs, 13 of them P1, all filed by `chatgpt-codex-connector` — no human review is unaddressed. ## Test plan Documentation only — no code changes, so `cargo` gates are unaffected. Branch state claims were produced from `git merge-tree --write-tree` and `git rev-list --count` per branch; CI and review-thread claims from the GitHub check-run and review-thread APIs; the `#1072` and `#1044` source claims by reading the files at those branches' heads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BAj1WWiTMvAJ6qK42LNyD6 --- _Generated by [Claude Code](https://claude.ai/code/session_01BAj1WWiTMvAJ6qK42LNyD6)_ Co-authored-by: Claude <noreply@anthropic.com>
Rewrites the 2026-09-01 sweep for the state at 2026-09-02 10:10 UTC, main at 44c9fa3 — seventeen commits later. #1089 covers the intervening window and is not repeated. The collision the first pass flagged has landed and taken the board with it. #1083, #1085, #1107 and #1108 all touched one seam and merged overnight; ten of the fifteen open PRs now conflict with main, where yesterday all thirteen merged cleanly. Nine of the ten conflict in some subset of egraph/graph.rs, egraph/saturate.rs, egraph/mod.rs and runtime.rs. The cheap mitigation was to rebase the queue after the first of the four landed, not after all four. Also records what became of the first pass's twelve recommendations — nine resolved, either merged or closed — and two corrections to it: - Thread count is not a quality signal. #1044 was ranked merge-ready partly on "zero unresolved threads", which was an artifact of the review bot hitting its usage limit before ever reading a 3,060-line diff. - The saturation collision had five participants, not three; #1044's variants.rs called a method #1085 deletes, in a disjoint file. A clean merge-tree is not evidence that a branch still builds. Closure recommendations for the three PRs that did not move: #1054 (red against main — X86Backend::epilogue removed by #1082, CompileError by #1081 — and invalidated by a second encoder refactor), #1072 (modify/delete against bootstrap_extraction_head.rs, which #1093 deleted, so it documents a harness main has removed), and a decide-either-way on #994. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jppittman
commented
Sep 2, 2026
Brought up to date with What broke it. The branch was green at The fix (
Gates run against current One thing still open, and a recommendation. This is now the second encoder refactor to invalidate this branch — the first, #1055–#1062's Vex-builder rewrite, is already in its own history at Generated by Claude Code |
fixed The remaining conflicted branches are not one undifferentiated pile. Each is two separable jobs, and conflating them is what made the set look intractable: (a) The core delta is superseded and mechanical. Every branch carries its own answer to "why did saturation stop", written before #1083 landed one. #1087 is the clearest: ten graph.rs hunks of `bool truncated` against main's `ScanStop { Completed, ClassCap, Deadline }` — the same fact at strictly more resolution, and this codebase's own "extend the type, not the convention" rule already applied on the main side. Nothing to weigh; take main's. (b) The harness collision is the real work. These branches add #[ignore]d measurement modules and main has since added its own in the same file regions. #1087's runtime.rs carries a single 456-vs-668-line hunk where its telemetry harness meets main's #1106 congruence probe, with near-duplicate helpers under different names. ~1,100 lines of test-only reconciliation per branch: it cannot break production, but it decides whether a published measurement reproduces. Also records that the five phase3 branches fork from a common merge-base with main but none is an ancestor of another — five independent experiments, no single resolution that templates them. #1109 is resolved and now merges clean; its story is kept as the template for job (a). Its alarming ~890-line runtime.rs conflict was both sides appending an independent module at EOF. The genuine work was porting its harness off the deleted env_extraction_policy onto Optimizer + Budget::Explicit, which keeps the caps as parameters the A/B needs. #1054's entry is rewritten from "recommend closing" to "fixed, rerun mutants then merge" — it was red and is now building, with the four deleted tests verified as targeting code that no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
#1109 reconciled, #1054 fixed and current, #994 brought to behind=0. Six of fifteen now merge cleanly and the one red CI is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
Records the structural finding this day's work produced, which matters more than any individual merge: reconciling a branch against the saturation seam has a half-life measured in hours. Evidence. The board went 5/15 clean to 7/15 as #1054, #1109 and #1087 were fixed, then back to 5/14 the moment #1087 merged — its landing re-conflicted #1109 and #1114, both clean an hour earlier. #1109 needed reconciling twice in one day for the same end-of-file append collision. #1114 was clean at 10:10 and needs an API port by 11:00 (Budget gained a field, SaturationStop::ClassCap changed arity, reused helpers went private). And the #1087/#1101 collision this document predicted materialized exactly as described. One mechanism behind all of it: egraph/graph.rs, egraph/saturate.rs and runtime.rs are one hot seam with ten branches queued on it, and each landing invalidates the rest. The fix is not more reconciliation — it is to rebase the queue as a batch after each seam landing, or freeze the seam until it drains. Corollary: reconcile in landing order and land promptly, because a reconciled-but-unlanded branch is a wasting asset. #1087 is the counter-example in the good direction, reconciled and merged the same hour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
Pulls the calls scattered through the document into one table: merge #1054 (after a mutants rerun) and #1072; rework #1109 rather than merging it as written, since its change is unbounded on a clock-free main — 353 seconds for a 279-node kernel — and should be paired with Budget::Applications and re-measured, with its stop-re-arming half landable separately; review #1113 and #1114 normally; decide #994 either way; hold the seven gated on the class-cap question. Records the superseded set for the record: #1050 and #1044, both closed and neither salvageable. Notes that #1054 looked like a third case and was not — it was twice-invalidated but its tests were re-targetable, and it is now green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NV5ZT2mxTnBC66unvBE7FF
Summary
Scheduled continuation of the test-quality-audit series
(
docs/bugs/*-test-quality-audit-followup.md). This pass picks uppixelflow-codegen/src/emit/x86_64.rs— the x86-64 SSE/VEX rawinstruction encoder — flagged since the 2026-08-08 audit as never
mutation-tested.
While this branch was in flight,
mainmerged eight commits (#1055–#1062)that rewrote the whole
emitmodule around a sharedVex/Digitbuilderabstraction and moved the collapse-loop driver logic into the per-ISA
files.
x86_64.rsgrew from 864 to 1647 lines. The first draft's ~35tests targeted functions that refactor deleted or reshaped, so this PR
merged
mainin and started the mutation pass over against the currentfile. Full arc documented in
docs/bugs/2026-08-31-test-quality-audit-followup.md.emit_xorps/emit_andps:pub fns with zero callers anywherein the repository.
Neg/Absalready go through the VEXemit_vxorps/emit_vandpsinstead.tests/collapse_loop.rs(an existing 11-test integration suite that compiles and executes
collapse-loop kernels), which turned out to already cover a whole
21-mutant category in
X86Backend's driver/scaffold plumbing —invisible only because this series has always scoped
cargo mutantsto
--lib. No unit tests were needed there; the audit doc adds amethodology note for future passes to check this before writing new
tests against a mutants report.
file's real API boundary (byte-in/byte-out for the encoder primitives,
plus two
frame_bytes > 0boundary conditions inX86Backend::prologue/epiloguethat are invisible to any value-comparison test since thewrong branch only emits dead-but-harmless bytes).
cargo mutants -p pixelflow-codegen --file .../x86_64.rs -- --lib --test collapse_loop: 388 mutants, 334 caught, 39 documented equivalent(
replace | with ^on bit-disjoint ModRM/VEX/REX fields — provablyidentical for every input), 0 real gaps.
Test plan
cargo test -p pixelflow-codegen(all targets incl. doctests andcollapse_loop.rs): passcargo build --workspace: cleancargo clippy -p pixelflow-codegen --lib --tests: cleancargo fmt -p pixelflow-codegen -- --check: cleancargo mutants -p pixelflow-codegen --file pixelflow-codegen/src/emit/x86_64.rs -- --lib --test collapse_loop: 388 mutants, 334 caught, 39 documented equivalent,0 real gaps
🤖 Generated with Claude Code
https://claude.ai/code/session_01BR5xrsVNGXHBceptcxBvui