Skip to content

ci: compile npu/forward's forward C++ as part of onebit, which the build job already builds (closes #898) - #912

Merged
bong-water-water-bong merged 1 commit into
mainfrom
pr/898-npu-forward-compile-check
Nov 3, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
pr/898-npu-forward-compile-check

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Root cause

.github/workflows/ci.yml:63-64 configures without -DONEBIT_NPU, so the build job compiles none of npu/forward's forward: npu/forward/src/q4nx_forward.cpp is only in the ONEBIT_NPU branch (CMakeLists.txt:802). Two neighbouring C sources in that directory are compiled, by tests — model.c (:641), q4nx_pack.c (:529, :667) — which is why the hole looked smaller than it is: the file that carries the math, and all five #808 A1 defects plus #874's missing q4nx_elf_arg_sizes declaration, was the one nobody built.

Why not "turn ONEBIT_NPU on", and not a stub either

ONEBIT_NPU needs the private NPU runtime (ONEBIT_NPU_RT_LIB/_RT_DIR), which the CI image does not have. But compiling it needs no runtime: the in-tree npu/rt.h is an alias of npu_rt_api.h behind ONEBIT_NPU_RUNTIME_OWN — no XRT, no private header, no stub that could drift from the real interface. (My own earlier note on this board said a stub rt.h was needed; that was wrong, and this PR is the measurement that says so.)

The change

  • onebit_npu_forward_syntax: an OBJECT library over npu/forward/src/q4nx_forward.cpp with the real target's include dirs (npu/forward/include, npu/forward/engine-src, npu), ONEBIT_NPU_RUNTIME_OWN, nlohmann_json and the same guarded OpenMP flags — so #pragma omp clauses are parsed exactly as in onebit_npu_forward. It links nothing.
  • add_dependencies(onebit onebit_npu_forward_syntax), so the existing CI build job (cmake --build build --target onebit …) compiles it.

No workflow file is touched: this box's gh token carries repo only, no workflow scope. If a lane that has that scope prefers it, adding onebit_npu_forward_syntax to ci.yml's explicit target list is a one-line follow-up; it would merely make the step visible by name in the log.

Measured (this tree, Release, gcc-16, on strixhalo)

configure                                   rc 0
ninja -n onebit                             -> [23/27] Building CXX object
                            CMakeFiles/onebit_npu_forward_syntax.dir/npu/forward/src/q4nx_forward.cpp.o
clean tree, ninja onebit_npu_forward_syntax  -> object built, rc 0
#808's rebase defect injected at :1894      -> FAILED:
  npu/forward/src/q4nx_forward.cpp:1894:77: error: invalid conversion from 'int' to
  'const char*' [-fpermissive]   (initializing argument 8 of `npu_gemm(I8Ctx&, const float*, int,
  int, const float*, std::vector<float>&, int, const char*, int)`)

The negative control is the defect verbatim: npu_gemm(..., 0, "attn_qkv", g->real_k) → "attn_qkv" replaced by g->real_k, which is what #808's rebase did at 16 call sites (7 of them that hard error). Restored before commit; the only files in this PR are CMakeLists.txt.

What this does not do

The same hole exists for the ONNX EP leg (ONEBIT_EP_NPU off — the #874 class) and it is not fixed here; npu/ort/onebit_npu_gemm.cpp still is not compiled by CI. That is a second, separate change.

closes #898

…uilds

The build job configures without ONEBIT_NPU (ci.yml:63-64), so
npu/forward/src/q4nx_forward.cpp - the file all five #808 A1 defects and #874's
missing declaration lived in - was never compiled by CI.

npu/rt.h is in the tree and needs no XRT: it is an alias of npu_rt_api.h behind
ONEBIT_NPU_RUNTIME_OWN, so a compile-only target needs no private runtime and no
stub header. onebit_npu_forward_syntax is an OBJECT library (it links nothing)
with the real target's include dirs, macro and OpenMP flags, and `onebit`
depends on it - so the CI build job, which builds `onebit`, compiles it with no
workflow change (this box's token carries no `workflow` scope).

Measured on this tree, release configure:
  ninja -n onebit  -> [23/27] Building CXX object
                     CMakeFiles/onebit_npu_forward_syntax.dir/npu/forward/src/q4nx_forward.cpp.o
  clean tree       -> object builds, rc 0
  #808's rebase defect injected at :1894 (an int where npu_gemm's 8th argument
  is `const char* sc`) -> FAILED, "error: invalid conversion from 'int' to
  'const char*' [-fpermissive]"

closes #898
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

808 - Partially compliant

Compliant requirements:

  • Split the XDNA launch layer behind NpuTarget interface
  • Move device and launch layer code to npu/target/xdna/ directory
  • Extract NpuTarget interface in npu/target/target.h
  • Update q4nx_forward.cpp to use npu_gemm() calls instead of direct kernel launches

Non-compliant requirements:

  • Ensure no numerics or kernel changes in this phase (this is a mechanical split, so no changes expected)

Requires further human verification:

  • Verification that the interface is correctly implemented and all launch calls are properly routed through NpuTarget

874 - Partially compliant

Compliant requirements:

  • Fix compilation error in ONNX EP NPU leg where q4nx_elf_arg_sizes is called without declaration

Non-compliant requirements:

  • None

Requires further human verification:

  • None

898 - Partially compliant

Compliant requirements:

  • Add a compile-only job that builds npu/forward/src/q4nx_forward.cpp to catch C++ errors in CI
  • Ensure that changes to npu/forward are compiled in CI

Non-compliant requirements:

  • None

Requires further human verification:

  • None
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing OpenMP Include

The find_package(OpenMP) call in CMakeLists.txt may not be sufficient to ensure OpenMP is properly enabled for the onebit_npu_forward_syntax library. If OpenMP is not available or not properly configured in the build environment, the #pragma omp clauses in the source code may not be processed correctly. This could lead to subtle performance issues or incorrect parallelization behavior.

find_package(OpenMP)
if(OpenMP_CXX_FOUND)
    target_link_libraries(onebit_npu_forward_syntax PRIVATE OpenMP::OpenMP_CXX)
endif()

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: READY at b06ab9d21 — reproduced the whole premise here, in a CI-shaped configure, including the negative control.

Reproduced (fresh configure exactly as ci.yml does it, i.e. without -DONEBIT_NPU):

cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16   rc 0
ninja -n onebit    → [23/27] Building CXX object CMakeFiles/onebit_npu_forward_syntax.dir/npu/forward/src/q4nx_forward.cpp.o
ninja onebit_npu_forward_syntax → built, rc 0, q4nx_forward.cpp.o 1,040,952 B

So add_dependencies(onebit …) really does put it on the path of the target the Build step names, with no workflow change.

Negative control, the #808 defect verbatim — :1894 "attn_qkv" → g->real_k, the substitution that broke 16 call sites in #808's rebase:

FAILED: [code=1] CMakeFiles/onebit_npu_forward_syntax.dir/npu/forward/src/q4nx_forward.cpp.o
npu/forward/src/q4nx_forward.cpp:1894:77: error: invalid conversion from 'int' to 'const char*' [-fpermissive]

Restored with git checkout -- afterwards; the tree I built from is the PR head.

The "it was the only uncovered file" claim, checked rather than accepted — the four C siblings are each compiled through the add_dependencies workaround, so the .cpp really was the hole:
q4nx_dequant.c → npu_dequant_bf16_test (:691, bound at :695), q4nx_semantics.c → npu_rmsnorm_offset_test (:701, bound at :706), q4nx_pack.c → npu_elf_args_test/npu_design_grid_test (:547/:685), model.c → npu_forward_loader_test (:659).

Two nits, neither a gate:

  1. The syntax target compiles a different branch of npu/rt.h than the shipped target. ONEBIT_NPU_RUNTIME_OWN is defined for it (CMakeLists.txt:370) and not for onebit_npu_forward (:815-839), and npu/rt.h:26 is #if !defined(ONEBIT_NPU_RUNTIME_OWN). That is what makes it buildable here without XRT, and it is right — but it means CI typechecks q4nx_forward.cpp against the in-tree API alias, not against the private runtime's header, so a signature drift between the two stays invisible. One clause in the comment block would stop someone reading this as "CI validates the file against the real runtime".
  2. NPU_INFER_DEBUG_DUMPS stays uncompiled (4 sites in this file; the real target defines it only under ONEBIT_NPU_DEBUG_DUMPS). Fine, but it is the one region of the file the new target still does not see.

Agreed with the body on the ONNX EP leg: npu/ort/onebit_npu_gemm.cpp is the same class of hole and is not covered here.

@bong-water-water-bong bong-water-water-bong added the tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied label Nov 3, 2026
@bong-water-water-bong
bong-water-water-bong merged commit ae0ee19 into main Nov 3, 2026
11 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the pr/898-npu-forward-compile-check branch November 3, 2026 17:54
bong-water-water-bong pushed a commit that referenced this pull request Nov 4, 2026
…x target compiles them

CI `build` on the merge ref has been red since #912 landed (engine#912's
`onebit_npu_forward_syntax` OBJECT target, main@363-379).  It mirrors
`onebit_npu_forward`'s include list -- `npu/forward/include`,
`npu/forward/engine-src`, `npu` -- but not the tree root, and P4 introduced
the only four root-relative quoted includes in the tree
(`npu/target/xdna/xdna_target.h` picks up `npu/target/target.h` and
`npu/plan/plan.h` the same way).  So the one TU CI added to catch defects in
this very file failed on its first include, with the merge ref red and the
branch green locally (this branch's base predates #912, so no local build
could see the target at all).

Fix: the four includes are npu-relative (`-Inpu`), which is how the rest of
the tree spells them (`q4nx_forward.h` already does it for `plan/plan.h`).

Spelling only, proven on the box: the merge ref's syntax TU compiles with no
tree root on the include path and yields the *same object* as the root-relative
form did (sha256 de57a84d8659…), and the ONEBIT_NPU=ON `1bit` rebuilt from
these sources is bit-identical to the binary that passed the #808 gate
(sha256 94cfe35745e5…, the B side in ~/p4-gate/tip3-lp-20261103T201138/head.txt).
The gate row therefore transfers to this commit without re-running it.
bong-water-water-bong pushed a commit that referenced this pull request Nov 4, 2026
…944 across the target split

Main moved under this branch four times after the 02:22 merge, and three of those commits
touch exactly the code P4 moved into npu/target, so the merge is a port, not a merge:

* #910 (gemv head) creates the lm head's context from a `full_i8_LMHEAD_*` ELF, tightens
  run_lm_head_gemv's tile-stream check (dtype by name, length must BE the padded or the real
  vocabulary width, zero-filled tail) and refuses the int8 tiled head when the family is gemv.
  Kept verbatim, with the context reached through the target (`xd_->make_head_ctx()`,
  `xd_->head_ctx()`, `xd_->head_ctx_ref()`, `xd_->ctx(i)`/`xd_->ctx16(i)`) instead of the
  members P4 moved.  The two functions that stayed in the model (`run_lm_head_gemv`,
  `run_lm_head`) now use the target's head context (`hctx`) where they used `ctx_head_`.
* #871 (fill a weight image straight into the BO) changed the two pb16i cache-miss sites from
  `make_weight_bo(*dev_)` to `make_weight_bo(*dev_, /*zero=*/false)`; those sites live in
  npu/target/xdna/xdna_target.cpp now, so the two-line delta is applied there.
* #944 (GDN head loop parallel) is outside the split and merged clean.

Resolution: 4 conflict hunks in npu/forward/src/q4nx_forward.cpp — the two npu_gemm bodies keep
P4's delegation to xd_->gemv(), the other two take main's text with the target accessors.  The
auto-merged init_impl block needed the same treatment by hand (it referenced the moved
`ctx_head_`), which is the reason this was resolved by reading, not by `git checkout`ing a side.

Verified: `onebit_npu_forward_syntax` compiles in the CI config (ONEBIT_NPU=OFF, the #912
target) and `onebit_npu_forward` compiles + links in ONEBIT_NPU=ON, which is the only place
npu/target/xdna/xdna_target.cpp is built.  tools/copyright.py --check rc 0.
bong-water-water-bong pushed a commit that referenced this pull request Nov 4, 2026
…988's C-sync rows land outside the split

main gained four commits; two of them edit the file this PR split:

  #988  3ea5d09  the C invalidate covers rows [0, last_am_) instead of the whole C BO, plus
                 I8Ctx::c_sync_print(); 4 lines in q4nx_forward.cpp
  #985  f9b19bb  the prefix-cache snapshot carries the GDN conv/recurrent planes whole and resumes
                 at cached-1; snapshot_state()/restore_state() in q4nx_forward.cpp, new
                 npu/forward/include/q4nx_state_planes.h

Both applied without a conflict, which is exactly the case this branch's history says to distrust, so the
merged result was read rather than trusted (that is how the family_tag, xor-rotation, head_pb16i_ and the
16 a_k/wkey/sc call sites were found — all in conflict-free or trivially-conflicting rebases):

  * #985 is model state, not launch state: q4nx_state::take_per_pos/take_whole in snapshot_state() and
    restore_state(), the include at the top, and the resume rule in app/npu_prefix_cache.h. Nothing it
    touches was moved into XdnaTarget, and the split leaves those two functions in the model.
  * #988 is inside I8Ctx (last_am_, c_host_bytes(), NPU_NO_C_NARROW) with one caller-side line, the
    NPU_C_SYNC_PROF print. The target neither writes kctx.Am (its only Am uses are the debug dumps at
    xdna_target.cpp:541,569,806) nor reads C outside [0, last_am_): row 0 for the single-row path, [0,M)
    after readback() for the rows path, which is the range the narrowed invalidate covers. So the split
    cannot inherit a stale row from that change, and it does not need to set activation rows itself.

Green on this boot: ninja (all targets) rc 0, ctest 79/79 (1 device skip: mtp_batch_all), copyright rc 0,
and CI's own object target onebit_npu_forward_syntax rebuilt from these sources (the #912 lesson). Gate rows
re-run against A = origin/main@9020f8c6, because a landing in npu/forward voids them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 3/5 tprs-ready TPRS: review verdict READY posted by the standing reviewer; merge gate satisfied

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI does not compile npu/forward at all (ONEBIT_NPU off), so a PR touching the NPU forward gets a green build without its C++ being built

1 participant