Skip to content

refactor: LLM AI Coding Agent audit — split monoliths, clean annotations#2

Merged
leafyoung merged 7 commits into
mainfrom
llm-refactor
May 11, 2026
Merged

refactor: LLM AI Coding Agent audit — split monoliths, clean annotations#2
leafyoung merged 7 commits into
mainfrom
llm-refactor

Conversation

@leafyoung

Copy link
Copy Markdown
Owner

Summary

Implements the refactor plan from llm-refactor-plan.md — an LLM AI Coding Agent audit of the codebase evaluating against predictable structure, flat architecture, and regenerability principles.

Changes

High Priority

  • Split compiled.rs (4,615 → 1,346 lines) into src/multi/backend/ module directory:
    types.rs, scalar.rs, simd.rs, wgpu.rs, device.rs, dispatch.rs, mock.rs
  • Decompose 1,083-line gradient_batch_auto_into — resolved automatically by moving per-backend logic into separate ExecutionBackend impls
  • Extract ExprGraph/ExprNode into src/multi/expr.rs (207 lines)
  • Extract Tape/TapeWorkspace into src/multi/tape.rs (565 lines)
  • graph.rs reduced from 6,315 → 5,562 lines

Cleanup

  • Replace per-item #[allow(unused_imports)] with module-level annotation in multi.rs
  • Remove #[allow(dead_code)] from public API items where clippy allows
  • Consolidate lib.rs re-exports into single pub use multi::{…} block
  • Add module-level docs to macros.rs
  • Fix trailing comma in mf4.rs macro invocation
  • Remove architecture comments from tests_comprehensive.rs

Verification

  • ✅ 863 tests pass (cargo test --all-features)
  • ✅ Clippy clean (cargo clippy --all-targets --all-features -- -D warnings)
  • cargo fmt --check clean
  • ✅ No logic changed — pure code movement and annotation cleanup

Remaining (not in scope)

  • Medium items 6–9 in the plan (unify Dual types, inline example files, split forward.rs)
  • Graph core impl is still ~2,500 lines — further splitting would require breaking impl Graph across files

leafyoung added 4 commits May 11, 2026 21:04
- Replace per-item #[allow(unused_imports)] with module-level in multi.rs
- Remove #[allow(dead_code)] from public API items where safe
- Add module-level docs to macros.rs
- Fix trailing comma in mf4.rs macro invocation
- Consolidate lib.rs re-exports into single pub use multi::{...} block
- Remove architecture comments from tests_comprehensive.rs
Move 3200+ lines from the monolithic src/multi/compiled.rs into focused
modules under src/multi/backend/:

  types.rs    — OpCode, FlatInstruction, BackendCapabilities, Instruction
  scalar.rs   — ScalarBackend
  simd.rs     — SimdBackend (f64x2, f64x4) with all SIMD helpers
  wgpu.rs     — WgpuBackend, WgpuBuffer, WgpuBufferSet (feature-gated)
  device.rs   — DeviceBatchPlan, DeviceBuffer*, transfer types, traits
  dispatch.rs — BackendKind, BackendSupportReport, auto-dispatch
  mock.rs     — MockDeviceBackend

compiled.rs retains only CompiledGraph, batch types, and workspace.
No logic changed — pure code movement with updated imports.

863 tests pass, clippy clean.
Move ExprGraph/ExprNode + operator overloads to src/multi/expr.rs (207 lines).
Move Tape/TapeWorkspace + compiled tape logic to src/multi/tape.rs (565 lines).
graph.rs goes from 6300 to 5562 lines.

863 tests pass, clippy clean.
Copilot AI review requested due to automatic review settings May 11, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the multi-variable autodiff implementation to improve structure and maintainability by splitting large modules into focused submodules (notably backend execution, expression graph construction, and tape evaluation), along with annotation and re-export cleanup.

Changes:

  • Split compiled execution/backends into src/multi/backend/* (scalar/SIMD/WGPU + planning/dispatch/types) and updated dispatch/re-exports accordingly.
  • Extracted expression-graph builder (ExprGraph/ExprNode) into src/multi/expr.rs and tape evaluation (Tape/TapeWorkspace) into src/multi/tape.rs.
  • Cleaned up annotations/re-exports and small hygiene fixes (macro docs added, trailing comma fix, comment cleanup in comprehensive tests).

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/tests_comprehensive.rs Removes architecture/testing-location comments from comprehensive tests.
src/multi/tape.rs New tape evaluation module extracted from graph.rs.
src/multi/multi_fn.rs Removes #[allow(dead_code)] annotations from public API items.
src/multi/graph.rs Removes inlined Tape/ExprGraph code and wires in extracted modules + backend imports.
src/multi/expr.rs New expression-graph module extracted from graph.rs.
src/multi/builder.rs Removes #[allow(dead_code)] on GraphBuilder::num_inputs.
src/multi/backend/wgpu.rs New WGPU backend module (feature-gated).
src/multi/backend/types.rs New backend core types/capabilities/opcode definitions.
src/multi/backend/simd.rs New SIMD backend module (x86_64 lanes) for batch compute/gradients.
src/multi/backend/scalar.rs New scalar backend module implementing the execution trait.
src/multi/backend/mod.rs New backend module root with submodule declarations and re-exports.
src/multi/backend/mock.rs New mock “device-style” backend executing on CPU with device plans.
src/multi/backend/dispatch.rs New backend kind + support reporting + dispatch helpers.
src/multi/backend/device.rs New device-oriented planning/buffer/transfer types and trait.
src/multi.rs Adds backend/expr/tape modules and restructures re-exports (plus module-level lint allow).
src/mono/mono_fn.rs Removes #[allow(dead_code)] annotations from public API items.
src/mono/mf4.rs Removes trailing comma in mono_ops! invocation.
src/macros.rs Adds module-level docs for macro APIs.
src/lib.rs Consolidates multi re-exports into a single pub use multi::{...} block.
llm-refactor-plan.md Adds the refactor plan document describing the audit and split strategy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/multi/tape.rs Outdated
Comment thread src/multi/backend/mock.rs Outdated
Comment thread src/multi.rs Outdated
Comment thread src/macros.rs Outdated
@leafyoung

Copy link
Copy Markdown
Owner Author

Addressed the unresolved Copilot review threads in c98b056:

  • removed unused GraphNode import from src/multi/tape.rs
  • removed unused DeviceBatchPlan import from src/multi/backend/mock.rs
  • removed the broad #![allow(unused_imports)] from src/multi.rs; kept only narrow per-reexport allowances where needed
  • corrected src/macros.rs docs to describe the macros as returning arrays, not Vecs
  • cleaned up a few additional unused imports exposed by removing the broad allowance (compiled.rs, graph.rs, wgpu.rs)

Validation:

  • cargo fmt
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all-features --lib
  • CI run 25678833475 is green

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.

Comment thread src/multi/backend/wgpu.rs
Comment thread src/multi/backend/wgpu.rs

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request performs a significant refactor of the petite-ad codebase, primarily focusing on splitting monolithic files like compiled.rs and graph.rs into modular backend components (scalar, SIMD, WGPU). It also introduces a new DeviceBackend trait and backend dispatch logic to improve architecture predictability. My review identified several performance concerns: the WGPU backend currently re-initializes the device context on every batch computation, which is highly inefficient. Additionally, the SIMD backend performs unnecessary heap allocations for derivative calculations and uses inefficient vector resizing inside performance-critical loops. I have suggested using fixed-size arrays and fill() for better performance.

Comment thread src/multi/backend/dispatch.rs
Comment thread src/multi/backend/dispatch.rs
Comment thread src/multi/backend/simd.rs Outdated
Comment thread src/multi/backend/simd.rs Outdated
Comment thread src/multi/backend/simd.rs Outdated
Comment thread src/multi/backend/simd.rs Outdated
Comment thread src/multi/backend/simd.rs Outdated
Comment thread src/multi/expr.rs
…sfers

GPU storage buffers use little-endian layout (WGSL spec).
Replace to_ne_bytes/from_ne_bytes with to_le_bytes/from_le_bytes
in encode_f64_slice, decode_f64_bytes, encode_f32_slice,
decode_f32_bytes, and encode_u32_slice to ensure correct
bit-pattern interpretation on big-endian hosts.
@leafyoung

Copy link
Copy Markdown
Owner Author

Fixed in e01c517: replaced all to_ne_bytes/from_ne_bytes with to_le_bytes/from_le_bytes in the five WGPU buffer encode/decode helpers (encode_f64_slice, decode_f64_bytes, encode_f32_slice, decode_f32_bytes, encode_u32_slice). GPU storage buffers use little-endian layout per the WGSL spec, so native-endian was incorrect on big-endian hosts.

…expr panic doc

simd.rs:
- simd_scalar_first_derivatives returns [f64; 2] instead of Vec<f64>,
  eliminating one heap allocation per derivative lane call
- pre-allocate cotangents with correct size before the batch loop
  (f64x2 and f64x4), replace clear()+resize_with() inside the loop
  with fill() to zero without reallocation

dispatch.rs:
- document that BackendKind::Wgpu creates a new device per call;
  direct callers to hold a WgpuBackend instance for repeated use

expr.rs:
- add # Panics doc to ExprNode::same_graph explaining that mixing
  nodes from different ExprGraph instances panics at runtime,
  which is inherited by all arithmetic operator overloads
@leafyoung

Copy link
Copy Markdown
Owner Author

Fixed all 7 threads in bddd223:

dispatch.rs (2 × HIGH — WgpuBackend per-call init)
Added # Performance doc notes to compute_batch and gradient_batch on BackendKind explaining that the Wgpu variant calls WgpuBackend::new_default() on every invocation and that callers should hold a WgpuBackend instance directly for repeated use. The root cause is BackendKind being a stateless enum; a stateful WGPU path through it will always bear this cost.

simd.rs (4 × MEDIUM — cotangent alloc / fill)

  • simd_scalar_first_derivatives now returns [f64; 2] instead of Vec<f64>, eliminating one heap allocation per derivative lane (unary ops return [dy, 0.0], binary ops return [dy_left, dy_right])
  • cotangents for both f64x2 and f64x4 backward passes is now allocated at full size before the batch loop (vec![zero; value_count]), and the loop body uses fill(zero) instead of clear()+resize_with()

expr.rs (1 × MEDIUM — panic documentation)
Added a # Panics doc section to ExprNode::same_graph explaining that mixing nodes from different ExprGraph instances panics, which is inherited by all arithmetic operator overloads (+, -, *, /).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.

@leafyoung
leafyoung merged commit 7cb69d3 into main May 11, 2026
8 checks passed
@leafyoung
leafyoung deleted the llm-refactor branch May 11, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants