[ATTN][BUILD] Make the chunk-prefill _b16 tile policy explicit in .conf files - #575
[ATTN][BUILD] Make the chunk-prefill _b16 tile policy explicit in .conf files#575baodii wants to merge 5 commits into
Conversation
…nf files Each chunk-prefill config line previously expanded to two translation units: the standard chunk_policy_head<N> and its _b16 variant. The _b16 policy is only ever dispatched when a request is paged and the KV page size is 16 (fmha_xe2.cpp: use_b16_policy = is_paged && block_size == 16), so every generated _b16 kernel with paged=false was dead code that could never be reached at runtime. Add an optional 7th field to the chunk-prefill config format: headsize,paged,causal,local,sink,lse[,b16] omitted -> both policies are generated (unchanged, backward compatible) b16=false -> chunk_policy_head<N> only b16=true -> chunk_policy_head<N>_b16 only Entries with b16=true and paged=false are unreachable and are now skipped with a warning. Rewrite chunk_prefill_default.conf to be fully explicit. All 35 standard policy kernels are preserved byte for byte, so the non-_b16 dispatch surface is unchanged. Of the 35 previously generated _b16 kernels, 14 were the undispatchable paged=false variants and 7 more covered shapes that are not exercised anywhere, leaving the 14 _b16 kernels backed by tests/flash_attn/test_flash_attn_varlen_func.py. Default chunk-prefill build drops from 70 to 49 kernels. chunk_prefill_full.conf is untouched and still generates 240. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bao <di.bao@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
The updated CMake config parser still accepts malformed field counts (2–5 fields or >7 fields) in ways that can silently expand to unintended kernel sets, and should reject these with a warning to avoid accidental large builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the chunk-prefill “_b16 tile policy” axis explicit in chunk-prefill .conf entries, aligning config readability with what actually gets compiled and reducing default-build AOT cost by avoiding undispatchable kernels.
Changes:
- Extend
chunk_prefill_configure.cmaketo support an optional 7thb16field, allowing explicit selection of standard vs_b16policy (and skippingb16=true,paged=falseas unreachable). - Update
chunk_prefill_default.confto be fully explicit (one kernel per line), reducing the default preset from 70 to 49 kernels. - Refresh documentation to describe the new
b16field and correct kernel-count tables.
File summaries
| File | Description |
|---|---|
KERNEL_CONFIGURATION.md |
Documents the new optional b16 field semantics and updates kernel-count tables. |
csrc/xpu/attn/xe_2/chunk_prefill_configure.cmake |
Adds parsing/validation and tuple generation for optional b16 policy selection. |
csrc/xpu/attn/kernel_configs/README.md |
Updates kernel counts and clarifies how b16 affects expansion to translation units. |
csrc/xpu/attn/kernel_configs/chunk_prefill_default.conf |
Makes all entries explicit and removes unreachable/dead _b16 kernels from the default preset. |
Review details
Suppressed comments (1)
csrc/xpu/attn/xe_2/chunk_prefill_configure.cmake:233
- Entries with 2–5 fields (e.g. a missing boolean) currently fall into the “no booleans specified” branch and expand into all 20 combinations for that head size. Given the documented format (either 1 field, 6 fields, or 7 fields), these partial entries should be treated as invalid and skipped with a warning to avoid accidentally compiling a large kernel matrix due to a typo.
"${b16_policy_${_headsize}}|${_paged}|${_causal}|${_local}|${_sink}|${_lse}"
)
endif()
else()
# No booleans specified: generate all 20 valid combinations.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The selective-mode parser branched on `_nparts GREATER_EQUAL 6`, so an entry with more than 7 comma-separated fields read the 7th as b16 and silently ignored the rest. The same branch also meant an entry with 2 to 5 fields fell through to the bare-headsize path and silently expanded into all 20 combinations. Both hide config typos behind an unexpected kernel set. Require exactly 1, 6 or 7 fields and warn plus skip otherwise. Also validate the boolean fields with quoted values and `foreach(... IN LISTS ...)`, so an empty field from a trailing or doubled comma is seen rather than dropped by unquoted list expansion. Previously `128,true,true,false,false,false,` parsed as an empty b16 and silently generated both tile policies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bao <di.bao@intel.com>
Treat b16 as an ordinary mandatory parameter rather than an optional 7th field, so a config line describes exactly one kernel with no hidden expansion. The selective-mode parser now requires _nparts to equal 7 and warns plus skips anything else. This drops the implicit forms that made the generated kernel set hard to predict from reading a .conf: a bare head size expanded to all 20 combinations in both tile policies (40 kernels from one line), and a 6-field line expanded to both policies (2 kernels from one line). Both are now rejected, and the emission path collapses to a single list(APPEND) selected by the b16 flag. chunk_prefill_default.conf is already fully explicit and chunk_prefill_full.conf uses the 'all' keyword, so both shipped presets are unaffected: 49 and 240 kernels with no warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bao <di.bao@intel.com>
The cmake-format pre-commit hook re-wraps comment text to fill the configured line width. Apply its output for the three comment blocks added by the previous commits so the hook is a no-op. Comments only; the generated kernel set is unchanged (default.conf still yields 49 TUs, full.conf 240, both with no warnings). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bao <di.bao@intel.com>
The 'all' keyword expanded the chunk-prefill matrix over every policy x paged combination, including the six _b16 policies with paged=false. fmha_xe2.cpp selects a _b16 policy only when is_paged && block_size == 16, so those 60 kernels can never be dispatched and were pure build cost. Apply the same b16 => paged rule the selective parser already enforces to the full-config branch. chunk_prefill_full.conf now emits 180 kernels instead of 240 (-25%) with no functional change; the removed set is exactly the 60 b16 + paged=false tuples. chunk_prefill_default.conf is unchanged at 49. This is the config used by the CI build-wheel job, which is on the critical path for the unit-test jobs. Docs and the full.conf header are updated to match, including the stale optional-field format description. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bao <di.bao@intel.com>
|
full build wo/ cache still take 5 hours in our CI:( |
Purpose
Every chunk-prefill config line silently expanded to two translation units:
chunk_policy_head<N>and its_b16variant (same thing withTileShapeQK[1]32 -> 16, sotiles_per_pagestays 1 at page size 16). But the dispatcher only selects_b16in one case (fmha_xe2.cpp:269):So every
_b16kernel withpaged=falsewas dead code: 14 of the 70 inchunk_prefill_default.conf, 60 of the 240 inchunk_prefill_full.conf, each paying full AOT cost across 4 devices. The tile policy was also invisible in the config, so a 35-line.confproducing 70 kernels was not apparent to a reader.This makes
b16a regular required field:All seven fields are mandatory and each line maps to exactly one kernel.
b16=falsebuilds the standard policy,b16=truebuilds_b16, andb16=truewithpaged=falseis rejected as undispatchable. The parser requires exactly 7 fields and warns plus skips otherwise, which removes the old implicit expansions that made the kernel set hard to predict from reading a.conf: a bare head size produced 40 kernels from one line, and a 6-field line produced 2.The same
b16 => pagedrule is applied to theallkeyword, sochunk_prefill_full.confdrops the 60 undispatchable tuples too. This is the config the CIbuild-wheeljob uses, and that job gatesrun-unit-tests-pvc/run-unit-tests-bmg.chunk_prefill_default.confis rewritten fully explicit. Paged decode already treats this dimension as a first-classpagesizefield, so this brings chunk prefill to the same footing.Test Plan
No unit test exists for the CMake parser and this box has no XPU hardware, so the parser was executed directly via a
cmake -Pharness that stubsconfigure_file()and prints the generated TU list. Verified the accepted and rejected forms, and diffed the emitted kernel sets old vs new.Test Result
chunk_prefill_default.confchunk_prefill_full.conf_b16dispatch surface is unchanged.full.conf, the emitted set was diffed old vs new: 60 removed, 0 added, and all 60 are_b16withpaged=false. Zero undispatchable kernels remain in either preset._b16TUs removed fromdefault.confthat no test exercises: head 96 paged (x3), head 192 paged (x2), head 256 paged+LSE, head 512 paged+LSE._b16TUs indefault.confare all backed bytest_flash_attn_varlen_func.py.file(STRINGS)splits on non-ASCII bytes).Malformed entries that used to be accepted silently are now rejected:
Both shipped presets are unaffected by the stricter parser:
chunk_prefill_default.confis already fully explicit andchunk_prefill_full.confuses theallkeyword.Please review carefully
This is a breaking change to the chunk-prefill config format. Any out-of-tree
.confusing a bare head size or the 6-field form will now warn and skip rather than build. Both in-tree presets are fine, but downstream configs need theb16field added.The 7 untested
_b16removals fromdefault.confare a real, if narrow, coverage reduction, andflash_attn_varlen_funcswallows"not compiled"errors into a PyTorch fallback, so a missing kernel is a slowdown rather than a failure. Page-16 coverage in the two default configs now disagrees:paged_decode_default.confhaspagesize=16for heads 64, 96, 128, 192, 256chunk_prefill_default.confhasb16=truefor heads 64, 128, 256, 512Heads 96 and 192 (Phi / Qwen3.5, DeepSeek MLA) would decode with a real kernel but prefill through the fallback at page size 16. Realigning costs 5 TUs (49 -> 54). Happy to add them back if you prefer symmetry over the smaller build.
The
full.confreduction carries no coverage risk, since the removed kernels are unreachable by construction.Unrelated finding, for visibility: the UTs exercise 108 distinct chunk-prefill tuples but only 38 exist in
chunk_prefill_default.conf. CI never notices because the unit-test jobs install the full-config wheel, whilebuild-wheel-with-default-configis built but never tested. That job is also 100% ccache hits today (measured: 761 cacheable calls, 761 hits, 0 misses), so thedefault.confreduction is a cold-build and binary-size win rather than a CI-time win. Thefull.confchange is the one that shortens the critical path. All of this predates the PR and is otherwise left alone.(Optional) Documentation Update
KERNEL_CONFIGURATION.md: documentedb16as a required field with thepaged=truerule and the strict field count; corrected the stale default-preset counts (claimed~31/~17, actual 49/32); updated the full-preset counts to 180.csrc/xpu/attn/kernel_configs/README.md: updated counts, replaced the now-wrong "each line expands to two kernels" note, documented whatallexpands to.chunk_prefill_configure.cmakeheader and both.confheaders describe the seven-field format;chunk_prefill_full.confhad a stale optional-field description that is now corrected.