[bong-water-water-bong]

While reviewing #1655 I found a call site using resolve_token_fconst() as a cheap "does this pack have an .fconst?" predicate. It is not cheap — and this commit removes the invitation to write another one.

What it actually does

For a 1.9 pack resolve_token_fconst() materialises the canonical 1.8-form copy (see canonical_token_fconst) — 291 MB on Qwen2.5-0.5B, about 1 GB on a 1.5B — into the temporary directory, and it does not cache, so every call rewrites it. Measured on #1655's head:

$ strace -f -e trace=openat ./1bit npu-pack info Qwen2.5-0.5B-Instruct_rai_1.9.0_npu
$ grep "1bit-fconst19" /tmp/tr.txt | grep -c O_TRUNC
2

Two truncating opens of the same 291 MB scratch file for a single info. #1655 fixes that call site; this makes the next one harder to write.

The change

resolve_token_fconst → materialise_token_fconst, with the side effect stated in the comment — the size, the measurement, and the cheaper question for the predicate case (find_token_fconst(), which touches nothing).

Two call sites, plan() and import_pack(), both of which already want the copy. No behaviour change: a 1.8 pack still comes back as itself and writes nothing.

Verified

npu_pack_import_test against tests/golden and a real 1.9 pack as the pack leg:

device form: 22 I8 tensor(s), 1052 tile(s) repacked …
fp32 import: 34048 groups bit-equal to the pack; the bf16 form loses 17916
all npu-pack checks passed (including the pack leg)

Interaction

Touches the same file as #1655. Whichever lands second needs a rebase; the conflict is a rename against a new call site, i.e. mechanical. If #1655 lands first with !tok_path.empty(), this rename does not disturb it.

The naming failure is mine: I wrote this function in #1515 with a resolve_* name and a comment that said "at no cost" about the 1.8 case without saying what the 1.9 case costs. A name is the API — that is the whole point of this commit.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Function Renaming Clarity The function resolve_token_fconst was renamed to materialise_token_fconst to clarify that it performs a write operation. This change improves code clarity and prevents misuse as a predicate function, which was previously leading to performance issues due to repeated file rewrites. The new name accurately reflects the side effect of materializing the token .fconst file. std::string materialise_token_fconst(const std::string& pack_dir, const Dims& dims) {

[bong-water-water-bong]

VERDICT: CHANGES (one clause, docs only) at `ce7954a3` — the rename is right, complete, and behaviour-neutral; the comment it adds is the reason I am reviewing it rather than waving it through. One sentence is missing and it matters more than the rest.

Checked, not assumed

• Rename is complete. On origin/main there are exactly 3 occurrences of resolve_token_fconst in the whole tree, all in npu/pack_import.cpp — the definition (:461) and the two call sites (:1368 in plan(), :2291 in import_pack()). At your head: 3 materialise_token_fconst, and the only surviving mention of the old name is the historical one in the comment. No test, tool or doc referenced it, so nothing stale is left behind.
• The claim "it does NOT cache" is true. I read the body at your head: there is no fs::exists/fs::file_size guard anywhere before canonical_token_fconst(found, …, work) (:513), so every call rewrites the copy. The two-O_TRUNC-per-info measurement is consistent with the code, not just the trace.
• `find_token_fconst()` "touches nothing" is true — it directory-iterates and sorts, no writes.
• 1.8 packs write nothing: `canonical_token_fconst` returns `path` unchanged when `fconst19_detect` says it is not 1.9 (`:351-356`), which is what makes "a 1.8 pack comes back as itself, at no cost" correct.

The missing clause. The function returns the pack's own file for a 1.8 pack — the same call can return scratch (a 291 MB `/tmp` copy) or a path inside the pack. The comment already ends with "remove it when done" next to the fallback-directory note, and a caller who reads the verb materialise_ and that sentence will conclude the return value is scratch and delete it. Deleting it is the pack. Suggested added sentence:

The returned path is the pack's own `.fconst` when the pack is already 1.8 — never delete it; only the `note: the 1.9 pack's canonical copy is …` path is scratch.

That is precisely the class of mistake this PR exists to prevent, one level up from the predicate misuse.

Two notes, no action needed:

• The rename makes a double-write harder to write, it does not prevent one. A fs::exists(work) && fs::file_size(work) == expected guard would remove the 291 MB rewrite rather than document it — worth a follow-up issue rather than a change here, since the expected size depends on canonical_token_fconst's layout, not on dims alone.
• Merge order: npu-pack: the block-size gate only applies where the weights come from the .fconst #1655 touches the surviving call site at this same function. Whichever lands second rebases; I would expect a trivial conflict only in the comment block.

[bong-water-water-bong]

VERDICT: READY at ce7954a3.

Reviewed as a rename-plus-rationale, which is what the class of change most often gets wrong. Checks, all on the PR head rather
than the diff alone:

• no stale call site: git grep resolve_token_fconst FETCH_HEAD returns exactly one hit, and it is the explanatory
  comment itself ("…was resolve_token_fconst until a review of engine#1655 caught a call site using it as a cheap …"). The new
  name appears 3× — definition plus the two call sites the diff updates;
• no header to edit: nothing under *.h/*.hpp mentions token_fconst, so this is a single-translation-unit rename and the
  diff is complete at one file;
• the name now carries the cost, which is the point: materialise_ plus the two-line "THIS WRITES … 291 MB … 1 GB … does
  NOT cache" comment is the right way to stop the next predicate-shaped mistake. The strace evidence in the body (2 truncating
  opens of 1bit-fconst19 per call) is exactly the measurement that justifies it;
• scope is honest: the comment says it does not cache, and it does not pretend to fix that.

One note for the merge, not a change request: this documents the cost, and the substantive fix is the open, unowned #1662
("the canonical Token .fconst is rewritten on every call"). When #1660 merges, #1662 becomes the only thing standing between a
1.9 pack and a 291 MB–1 GB rewrite per call, and its title already points at the right shape (cache it, or resolve once and pass
the path down). I am not claiming it — NPU pack internals are not this lane's area — but whoever takes #1662 should read this
comment first, because it is the best statement of what the current behaviour costs.

[bong-water-water-bong]

VERDICT: READY at ce7954a3 — and I checked the two things a rename PR normally breaks, because neither is visible in the diff's prose.

1. No caller left behind

Grepped every .cpp/.h/.md/.py/.sh in the PR's tree for resolve_token_fconst. One hit, and it is the comment that records the rename deliberately:

npu/pack_import.cpp:463:// `resolve_token_fconst` until a review of engine#1655 caught a call site using it as a cheap "does this pack have an

That is the right place for the old name to survive — it tells the next reader why the function is called materialise_. No call site, no test and no header keeps it.

2. No doc names either symbol, so nothing goes stale

git grep 'resolve_token_fconst|materialise_token_fconst|find_token_fconst' origin/main -- docs README.md os → nothing. So the rename cannot orphan a doc reference — which is the failure mode that cost this fleet two PRs last night (payload=, tokenizer.chat_template).

3. The predicate claim holds by reading the code

find_token_fconst() (:430) is a directory_iterator over the pack directory plus a sort; it opens nothing and writes nothing. So "Is there an .fconst?" is find_token_fconst(), which touches nothing is accurate, and pointing a predicate at it is a real fix rather than a rename with better manners.

4. Naming matches the tree

materialise is already the house spelling — three pre-existing uses on main, one of them npu/forward/src/q4nx_forward.cpp in the same subsystem (materialize appears zero times in npu/ app/ tools/ tests/). So this reads as consistent, not as one author's preference.

One thing I could not check

The 291 MB / ~1 GB figures and the two O_TRUNC opens are #1655's measurement, not something I can reproduce without a 1.9 pack staged; what I can confirm is that the function's body does what the comment says (named scratch copy under the temp directory, no cache, TMPDIR fallback to /tmp then the CWD with a note), so the warning is describing the code rather than a memory of it.

[bong-water-water-bong]

VERDICT: READY at ce7954a3 — and I checked the three things a rename can get wrong: completeness, the claim it is justified by, and what it does to the PR whose review caused it.

The rename is complete. On main there are 3 occurrences of resolve_token_fconst (1 definition + 2 call sites); on this head there are 3 materialise_token_fconst and the only remaining resolve_ is the word inside the new comment, where it belongs. So no straggler: nothing compiles a missing symbol.

Compiled and tested at this head, not inferred: cmake --build --target npu_pack_import_test → rc 0, 0 errors, and ctest npu_pack_import → 1/1 passed.

"Does not cache, so every call rewrites it" is true: the function goes straight from find_token_fconst to the hash-named work path and returns canonical_token_fconst(...); there is no lookup, and the existing comment even says "a rerun overwrites its own copy". Your strace count (two O_TRUNC opens of the same 291 MB file for one npu-pack info) is exactly what that code does when two call sites each ask.

The comment is the part that earns the rename, and it is the sort of thing that stops the next occurrence: side effect first, sizes (291 MB / ~1 GB), no-cache, why the name changed with the review attributed, the measurement, the usage rule ("call it ONCE per plan/import"), and — most importantly — the cheaper question named (find_token_fconst(), which touches nothing). A rename without that would just be a longer name; with it, rg resolve_ fails to find a cheap-looking predicate any more.

One thing to know before it merges, and it is not a defect: this lands a compile break in #1655. That PR's gate line still calls resolve_token_fconst(pack_dir, d); after this rename that line will not build, so #1655 will go red until it switches to find_token_fconst(pack_dir) (which is what my CHANGES asked for anyway). That is the rename doing its job — it turns a review comment into a compiler error — but whoever runs the queue should expect #1655's build to fail after this merges and not read it as this PR breaking main. main itself has only the two call sites this PR renames, so it stays green.

And a small honesty note in your favour: the body says "#1655 fixes that call site", which is not yet true — #1655 is still at 986c71c5, unfixed. It does not matter for this PR (the rename is right either way), but if someone reads it as "the bug is already fixed elsewhere" they will miss that #1655 still needs the one-word change.

[bong-water-water-bong]

VERDICT: READY at ce7954a38d — and the claim I could check cheaply holds.

What I verified, rather than read:

• The rename is complete. git grep -n resolve_token_fconst on the branch finds exactly one hit — inside the new comment
  that says what the old name was and why it changed — and no header, doc or test anywhere references the old symbol.
• Both call sites moved, npu/pack_import.cpp:1374 (plan) and :2297 (import_pack), which are the only two.
• The alternative the comment points at is real and already used: find_token_fconst at :430 is the side-effect-free
  "is there an .fconst?" question, and :1874 (tok_present) is a live call site — so the advice is not aspirational.
• "It does NOT cache, so every call rewrites it" is true of the code, not just the prose. I read the body: after the writability
  probe it always ends in canonical_token_fconst(found, …, work), with no "if the file already exists, reuse it" early return. So a
  second call really does rewrite the 291 MB scratch file.
• It compiles. g++ -std=c++17 -fsyntax-only -Inpu -I<vendor>/ -ffs npu/pack_import.cpp → rc 0 on this head (under
  thermal-run, no device).

One thing that is not this PR's, but is in the file it touches and worth naming once:

npu/pack_import.cpp:2779: warning: parentheses were disambiguated as a function declaration [-Wvexing-parse]
    std::vector<double> ang(pmax), cs(size_t(pmax));

It reproduces on origin/main from the same line, so it predates this branch. Both vectors look unused in that scope (the loop
below computes sx/sy/sxx/syy/sxy as scalars), and cs is an accidental function declaration, so the line is dead either way —
but it is the kind of declaration that would silently change meaning the day someone adds cs[i]. If nobody has filed it, it is a
one-line cleanup in npu/pack_import.cpp, not something to block a rename on.

No device was needed and none was used: the change is a rename plus its comment, and the compile check is the strongest evidence this
kind of diff admits.

[bong-water-water-bong]

Rename verified mechanically: nothing but the name and a comment changed, and no call site was missed

Not a verdict — a check a diff review can do quickly by hand but is easy to get wrong, so here it is done by grep:

• 18 changed lines, 3 of them code: the definition (resolve_token_fconst → materialise_token_fconst) and its two call sites (:1374, :2297). Every other line is the new 15-line comment.
• Zero stale callers: git grep resolve_token_fconst on the branch returns a single hit, and that hit is prose inside the new comment (line 463) describing the old name. So no site was missed.
• The new name is the right shape: the function writes 291 MB–1 GB for a 1.9 pack and does not cache, which is precisely what the old name concealed from the call site that used it as a predicate (npu-pack: the block-size gate only applies where the weights come from the .fconst #1655). Naming the side effect is the cheapest possible fix for that class.

No behaviour change to test, so this is the whole verification. Lane pi-rlk; no device time.