feat(qli): add radix-select TopK demo with validated cases and known-issue notes - #78
feat(qli): add radix-select TopK demo with validated cases and known-issue notes#78zzzyh222 wants to merge 13 commits into
Conversation
cbe944f to
8f97c69
Compare
💡 Codex Review Load transposed K through a column-major view
When SuperNPUBench/benchmark/one-level-arch/kernels/qli/qli_pto_opt.hpp Lines 657 to 663 in cbe944f Defer boundary candidates until all chunks are scanned
For multi-chunk inputs, this emits Parameterize golden dimensions instead of only renaming output
The documented large-case workflow passes only an SuperNPUBench/benchmark/one-level-arch/kernels/qli/qli_pto.hpp Lines 386 to 389 in cbe944f  Restrict the final baseline TopK load to valid columns
When Reject truncated matmul dumps before reporting success
When the dump is empty or contains fewer than SuperNPUBench/benchmark/one-level-arch/test/kernel/matmul/src/gfrun_matmul.py Lines 449 to 453 in cbe944f Return a failing status when numerical checks fail
After collecting failed runs or comparisons in Keep the requested RMSNorm epsilon in sync with the ELF
For Build the RMSNorm ELF with result checking enabled
The supplied build path omits Wire the advertised FIXP global dimensions into the benchmark
Only Detect every IEEE-754 NaN before building radix keys
This comparison recognizes only the single positive canonical bit pattern ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…line, golden dim args - qli_pto_opt.hpp: extraction now emits all key>kth_value across chunks in a first pass, then fills remaining slots from key==kth_value equal candidates (prevents out-of-range writes when early chunks hold many equal-boundary values while later chunks hold larger keys). - qli_pto_opt.hpp: NaN detection widened to 'exponent all-ones AND mantissa nonzero' so negative/signaling/payload NaNs map to the lowest sortable key instead of only the single canonical 0x7FC00000 pattern. - qli_pto.hpp: Skv>2048 tail chunk loads use physical padding tile with ValidCols=tail length, avoiding out-of-bounds reads and >Skv indices. - gen_qli_golden.py: dimensions (--sq/--skv/--topk/--g/--d) are CLI args; the output directory no longer drives tensor shapes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3dc77e526
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| python3 gen_qli_golden.py --mode gen \ | ||
| --outdir ../../../../../compare/qli_fp8_B1_Sq4_Skv8192_g64_Tm16_Tk32 |
There was a problem hiding this comment.
This command only changes the output directory name; gen_qli_golden.py explicitly does not infer dimensions from that name and defaults SKV to 2080. Following the documented 8192-token flow therefore creates undersized K/scale data and 2080-column references, while the later build instantiates Skv=8192, causing the kernel to read beyond the embedded inputs and making verification fail or report invalid experiment results. Pass --skv 8192 (and ideally all dimensions) here.
Useful? React with 👍 / 👎.
| .section .data | ||
| .global _binary_srcq_data_start | ||
| _binary_srcq_data_start: | ||
| .incbin "/home/z00947698/superScalar/SuperNPUBench/benchmark/one-level-arch/compare/qli_fp8_B1_Sq64_Skv128_g64_Tm16_Tk32/srcq.bin" |
There was a problem hiding this comment.
On any checkout that does not have this exact /home/z00947698/... directory, assembling qli_check_data.s fails before the advertised demo can build. The documented sed command only replaces the configuration-directory suffix, so it leaves this machine-specific prefix intact; moreover, the preceding generation command writes relative to the current checkout. Use a repository-relative/configurable path or generate this assembly file from the selected data directory.
Useful? React with 👍 / 👎.
| // indices 紧随 scores 之后，避免大 Sq*Skv 时与 scores 区域重叠 | ||
| #define OUT_INDICES (0x4000802000ULL + (uint64_t)Sq * Skv * 4) | ||
|
|
||
| #define B 1 |
There was a problem hiding this comment.
When this test is built with B other than 1, the Makefile passes -DBatch=$(B) and names the ELF accordingly, but this unconditional definition keeps both execution loops at one iteration. A nominal B=2 run therefore computes and emits results only for the first batch even if matching two-batch input data is embedded; derive B from Batch as is done for the other configurable dimensions.
Useful? React with 👍 / 👎.
| if constexpr (Skv <= MaxTileCol) { | ||
| using tile_s = Tile<Location::Vec, float, 1, Skv, BLayout::RowMajor>; | ||
| using tile_idx = Tile<Location::Vec, uint32_t, 1, 32, BLayout::RowMajor, 1, 1>; |
There was a problem hiding this comment.
For any supported Skv below 128 that is still a multiple of 8, this branch creates a physical FP32 tile smaller than the documented 512-byte TLOAD/TSTORE minimum. The radix implementation already handles the same case with SinglePhy = max(Skv, 128) and a smaller valid width, but the baseline will instead produce an invalid tile for configurations such as Skv=64; apply the same physical padding here or reject those sizes.
Useful? React with 👍 / 👎.
| uint32_t* key_scratch = reinterpret_cast<uint32_t*>( | ||
| reinterpret_cast<uint8_t*>(indices_gm) + (uint64_t)Sq * topK * 4 + 8192); | ||
| uint32_t* temp_hist = key_scratch + (uint64_t)Sq * Skv; | ||
| uint32_t* prefix_buf = temp_hist + 256; |
There was a problem hiding this comment.
When an integrator allocates only the documented [Sq, topK] indices output, this function unconditionally derives an undocumented scratch area beyond that allocation and writes Sq*Skv keys plus histogram and prefix storage there. The extra 8192-byte offset does not make the memory valid, so ordinary header use corrupts whatever allocation follows indices_gm; add an explicit workspace pointer/size or otherwise make the required storage part of the public buffer contract.
Useful? React with 👍 / 👎.
| # OUT_SCORES=0x4000802000, OUT_INDICES=OUT_SCORES+Sq*Skv*4（驱动内已动态化） | ||
| $SSM/bin/gfrun -f $ELF \ | ||
| --dump-memory 0x4000802000:$((Sq*Skv*4 + Sq*topk*4 + 65536)):$OUTDIR/sim_out_radix.bin |
There was a problem hiding this comment.
The documented shell flow never assigns Sq, Skv, topk, or OUTDIR before this command. In a normal shell, the unset arithmetic variables evaluate to zero, so the advertised 8192-token case dumps only 65536 bytes even though its score matrix alone occupies 131072 bytes; the subsequent reshape then receives truncated data. Define the variables in the snippet or use the literal dimensions selected in the preceding build command.
Useful? React with 👍 / 👎.
…ore B) - QLI_RADIX_POP_N: replace TROWMAX+TSTORE+TEXPANDS+TCMP+TMUL+TROWMAX index-derivation chain with TROWARGMAX(best) + TADDS(+chunkBase) - QLI_RADIX_POP_EQN: same TROWARGMAX on rev+1 (smallest global index) - drop unnecessary descending sort (TopK set contract) - verified: 4 key cases (Sq64/128, Sq4/2048/8192/2080) cosine=1.0 set=100%; adversarial early-eq+later-gt case PASS; multi-pattern NaN PASS - blocks Sq4/Skv2048: 71,918 -> 61,678 (-14.2%)
… extract) - A1: TROWARGMAX single-op pop; 4 key cases cosine=1.0 set=100%; blocks -14% - A2: bucket-reuse analysis shows no gain (skipped; docs record why) - B: TSCAN instruction implemented+verified (SSM + tileop-api; prefix-sum correct); TSCATTER blocked by LinxV5 'Match Instruction Error' -> B not adoptable; SSM no regression confirmed
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aed07713ca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { gidx gi(prefix_buf); TLOAD(idxTile, gi); } | ||
|
|
||
| th hist; | ||
| THISTOGRAMX(hist, key, idxTile, r); |
There was a problem hiding this comment.
On the repository's documented SuperScalarModel environment, this multi-round call cannot distinguish Byte2/Byte1/Byte0: the newly added qli_radix_issues_found.md states that the model decodes every ByteId as Byte3 and that the validating runs used a local, still-unsubmitted Block.cpp fix. Consequently rounds r=2..0 repeatedly histogram the highest byte, producing an incorrect threshold and TopK set for ordinary inputs whose candidates share that byte. Land the paired model fix or avoid relying on the broken byte selector before advertising this demo as runnable.
Useful? React with 👍 / 👎.
| using t1 = Tile<Location::Vec, RU, 1, 32, BLayout::RowMajor, 1, 1>; | ||
| using gi1 = global_tensor<RU, RowMajor<1, 1>>; | ||
| t1 s; | ||
| TROWSUM(s, m01); |
There was a problem hiding this comment.
On the stock documented SuperScalarModel, extraction aborts when this UINT32 mask reaches TROWSUM; the added design document explicitly says the successful runs used a local B8 validator change that permits UINT32/UINT16 for TROWSUM, TROWMAX, and TROWARGMAX, but that change is not part of this repository or the setup instructions. Even with the ByteId decoder corrected, every nonempty TopK invocation reaches this count operation and cannot run in the advertised environment, so either include the paired model update or implement the count/pop path with supported dtypes.
Useful? React with 👍 / 👎.
| volatile uint32_t* row_u32 = reinterpret_cast<volatile uint32_t*>(scores_gm + (uint64_t)i * Skv); | ||
| row_u32[idx_val] = 0xF149F2CAu; |
There was a problem hiding this comment.
When topK > 1 and a row contains scores below -1e30f, replacing the first selected value with this bit pattern makes that position larger than the remaining candidates, so TROWARGMAX repeatedly emits the same index instead of distinct TopK entries. The QLI API does not bound score magnitude, and FP32 inputs/scales can legitimately produce values below this approximation; track selected positions with a mask or use a representation that is guaranteed to rank below every supported score.
Useful? React with 👍 / 👎.
| tk tSign; TANDS(tSign, key0, FP32_SIGN_MASK); | ||
| tk tNeg; TNOT(tNeg, key0); | ||
| tk tPos; TORS(tPos, key0, FP32_SIGN_MASK); | ||
| tk tSign01; TSHRS(tSign01, tSign, static_cast<RU>(31)); | ||
| tk tDiff; TSUB(tDiff, tNeg, tPos); | ||
| TMUL(tDiff, tDiff, tSign01); | ||
| tk key; TADD(key, tPos, tDiff); |
There was a problem hiding this comment.
When +0.0f and -0.0f straddle the TopK boundary, this sign-based mapping assigns them different keys (0x80000000 versus 0x7fffffff) even though they are equal FP32 scores. The documented contract says equal scores are resolved by smallest index, so a later-index +0.0f is incorrectly selected ahead of an earlier -0.0f; canonicalize both zero encodings before the sortable-key transform so the equality extraction applies the promised index tie-break.
Useful? React with 👍 / 👎.
…iver batch; document sim deps/scratch contract Address Codex PR#78 review comments: - qli_pto.hpp: pad Skv<128 baseline tile to SinglePhy / 128 (v5 512B min); replace -1e30f sentinel with -FLT_MAX (0xFF7FFFFF) so scores below -1e30 cannot be re-selected. - qli_pto_opt.hpp: canonicalize -0.0f to +0.0f in radix sortable-key so equal zero scores honor smallest-index tie-break. - qli_check_opt.cpp: derive batch from -DBatch instead of hardcoding B=1. - test/kernel/qli/README.md: pass explicit --sq/--skv/--topk to golden gen; define dump-size vars; document SuperScalarModel local-fix requirements (ByteId decode, UINT32 reduce/argmax gates, TROWARGMAX mapping) and the scratch-memory contract for qli_topk_radix. Verified: 4 key cases (64/128, 4/2048, 4/8192, 4/2080) cosine=1.0 set=100%; sentinel adversarial case topK [0,1] distinct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 631a36cfb6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| *out = static_cast<int32_t>(idx_val); | ||
|
|
||
| volatile uint32_t* row_u32 = reinterpret_cast<volatile uint32_t*>(scores_gm + (uint64_t)i * Skv); | ||
| row_u32[idx_val] = 0xFF7FFFFFu; |
There was a problem hiding this comment.
When the requested TopK reaches an input scored -inf, replacing a selected entry with 0xFF7FFFFF (-FLT_MAX) makes that entry larger than the remaining -inf candidates, so the next TROWARGMAX selects the same index again; the chunked branch repeats the same write at line 467. Fresh evidence after the earlier sentinel report is that the follow-up changed the sentinel to -FLT_MAX, which fixes finite FP32 values but still does not rank below negative infinity; use an explicit selected-position mask or otherwise exclude prior indices.
Useful? React with 👍 / 👎.
|
|
||
| # scale_q: [Sq*g] FP32 — per-token-head Q 量化 scale | ||
| np.random.seed(SEED) | ||
| scaleQ = (np.random.randn(sq_ * g_) * 0.01).astype(np.float32) |
There was a problem hiding this comment.
The documented input contract declares scale_q and scale_k nonnegative, but this zero-mean Gaussian makes roughly half of scale_q negative, and line 90 does the same for scale_k. Consequently all advertised validation cases exercise sign-flipping values that cannot represent quantization scales, changing both score signs and TopK ordering relative to production-shaped inputs; generate positive scales, for example from absolute values or a positive distribution.
Useful? React with 👍 / 👎.
| .global _binary_srcsq_data_end | ||
| _binary_srcsq_data_end: | ||
| .global _binary_srcsq_data_size | ||
| .equ _binary_srcsq_data_size, .-_binary_srcq_data_start |
There was a problem hiding this comment.
Any consumer of _binary_srcsq_data_size receives the distance from the beginning of srcq, including all intervening Q/K/W data, rather than the size of srcsq; _binary_srcsk_data_size repeats the same error at line 44. Although the current driver only reads the start symbols, these exported size symbols will cause bounds checks, copies, or future generated-address code to overrun the corresponding blobs; subtract _binary_srcsq_data_start and _binary_srcsk_data_start, respectively.
Useful? React with 👍 / 👎.
| nm_path = sys.argv[3] | ||
|
|
||
| # Get symbol addresses from ELF | ||
| result = subprocess.run([nm_path, elf_path], capture_output=True, text=True) |
There was a problem hiding this comment.
If llvm-nm exits unsuccessfully, is pointed at the wrong ELF, or omits even one expected symbol, this unchecked invocation leaves some or all hard-coded SRC*_ADDR values unchanged and the script still prints Addresses updated. Because the documented workflow relies on these absolute addresses, the rebuilt demo can silently read stale locations and report invalid scores; check the subprocess return code and require all five symbols before rewriting the source.
Useful? React with 👍 / 👎.
Concise summary of QLI's current implementation (radix-select TopK), precision results, performance analysis, known issues, and future optimization directions. Replaces verbose qli_fix_record.md history as the canonical current-status reference.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd26de9dc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sed -i "s|/home/z00947698/SuperNPUBench|$REPO_ROOT|g; \ | ||
| s|qi_fp8_B1_Sq[0-9]*_Skv[0-9]*_g64_Tm16_Tk[0-9]*|qli_fp8_B1_Sq${SQ}_Skv${SKV}_g64_Tm16_Tk${TK}|g" \ |
There was a problem hiding this comment.
Fresh evidence after the earlier developer-path report is that the new replacement command still matches neither part of the committed path: the prefix in qli_check_data.s contains /home/z00947698/superScalar/SuperNPUBench, while this pattern omits /superScalar, and the directory starts with qli_fp8 while the second pattern starts with qi_fp8. Following Step 2 therefore leaves all five developer-specific paths unchanged, so assembly fails on other checkouts and also cannot switch to the selected 8192-token dataset.
Useful? React with 👍 / 👎.
| #ifndef B | ||
| #define B 1 |
There was a problem hiding this comment.
Fresh evidence after the earlier batch-count report is that the attempted guard checks B, but the qli_check_opt Makefile still passes -DBatch=$(B). Thus a documented build with B=2 leaves B undefined here, defines it as 1, and both execution loops process only the first batch; initialize B from Batch or have the Makefile define B.
Useful? React with 👍 / 👎.
| data = open('sim_out_radix.bin', 'rb').read() | ||
| scores = np.frombuffer(data[:SQ*SKV*4], dtype=np.float32).reshape(SQ, SKV) | ||
| indices = np.frombuffer(data[SQ*SKV*4:SQ*SKV*4+SQ*TOPK*4], | ||
| dtype=np.int32).reshape(SQ, TOPK) | ||
| ref = np.fromfile('reference_scores.bin', dtype=np.float32).reshape(SQ, SKV) | ||
| ref_idx = np.fromfile('reference_indices.bin', dtype=np.int32).reshape(SQ, TOPK) |
There was a problem hiding this comment.
When the documented steps are followed, gfrun writes sim_out_radix.bin to ${OUTDIR} and the generator also writes both reference files there, but this verification snippet opens all three as bare filenames without first changing into that directory. From the test/kernel/qli working directory established by the build steps, validation therefore raises FileNotFoundError; pass OUTDIR into the Python block or open ${OUTDIR}/... explicitly.
Useful? React with 👍 / 👎.
| ifeq ($(QLI_DTYPE), INT8) | ||
| DEFINES += -DQLI_INT8 | ||
| DT_TAG = _int8 |
There was a problem hiding this comment.
When QLI_DTYPE=INT8 is selected for the main qli_check_opt testcase, this branch makes the driver interpret srcq.bin and srck.bin as signed integers, but the only supplied generator always serializes FP8 E4M3 bit patterns and computes its reference from their FP8 values. The build therefore succeeds under an _int8 name while executing and validating unrelated numeric inputs, which can produce misleading experiment results; either add an INT8 generation/reference path or reject this configuration.
Useful? React with 👍 / 👎.
New file qli_pto_opt_simple.hpp: simplified MSD radix-select TopK with ~235 lines (vs ~376 in qli_pto_opt.hpp). Key simplifications: - FOR_EACH_CHUNK macro eliminates 4× repeated tail branching - MAKEKEY: sign-flip only (no NaN/-0 detection, ~7 tile ops) - Single POP_N macro (no separate POP_EQN with rev trick) - Merged GT+EQ extraction in single chunk loop - Code is clean enough to see the actual cost (4×NumChunks THISTOGRAM + topK×POP) qli_pto_opt.hpp retained as full-featured version (THISTOGRAMX asm, NaN/-0 defense, rev min-index extraction). Verified: Sq=64/128/128, Sq=4/2048/512, Sq=4/8192/512, Sq=4/128/8, Sq=4/2080/512 all cosine=1.0 set=100% nan=0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc821b19d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| #define OUT_SCORES 0x4000802000ULL | ||
| // indices 紧随 scores 之后，避免大 Sq*Skv 时与 scores 区域重叠 | ||
| #define OUT_INDICES (0x4000802000ULL + (uint64_t)Sq * Skv * 4) |
There was a problem hiding this comment.
When the driver runs with B > 1—for example after correcting the already-reported Batch macro mismatch or when compiling with -DB=2—the score loop writes B * Sq * Skv floats, but OUT_INDICES begins after only one batch. The first TopK call therefore overwrites batch 1's score matrix before its call at lines 98–101 reads it, producing invalid indices for subsequent batches; offset the indices region by B * Sq * Skv * 4 bytes.
Useful? React with 👍 / 👎.
| tk sign; TANDS(sign, bits, 0x80000000u); \ | ||
| tk neg; TNOT(neg, bits); \ | ||
| tk pos; TORS(pos, bits, 0x80000000u); \ |
There was a problem hiding this comment.
When +0.0f and -0.0f straddle the TopK boundary, this mapping assigns them different keys (0x80000000 and 0x7fffffff), so a later +0.0f can displace an earlier equal -0.0f. Fresh evidence after the earlier signed-zero report is that the full variant was corrected, but the actual demo driver now includes this simplified header, where the uncanonicalized mapping remains; normalize both zero encodings before applying the sign transform.
Useful? React with 👍 / 👎.
Replace all macros (FOR_EACH_CHUNK, MAKEKEY, HCHUNK, POP_N, EXTRACT) with template functions (RadixMakeKey, RadixChunkHist, RadixPopN, RadixExtract, HistMergeChunk, ExtractChunk). Main function uses direct if-constexpr for tail handling instead of FOR_EACH_CHUNK macro. - No macros, no lambdas, no ic wrappers - Clean template functions taking raw pointers + compile-time shape params - 207 lines for TopK section (vs ~235 with macros) - All 5 key cases verified: cosine=1.0 set=100% nan=0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6c3b3aa66
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| template <int CK, int CKV> | ||
| inline void RadixPopN(TKey<CK, CKV>& mv, RU chunkBase, int32_t* out, int n) { |
There was a problem hiding this comment.
When the recommended qli_check_opt target is built, it includes this simplified header, and every topK > 0 extraction eventually passes a candidate tile by reference to this function. The added qli_radix_issues_found.md documents that this exact RadixPopN function form is mis-lowered by the LinxV5 backend into an S64 load and an incompatible UINT32/INT64 comparison, causing validation to abort; qli_pto_opt.hpp therefore moved the operation into macros with function-local tiles, but the demo's simplified header retains the broken form. Apply the same local-tile workaround here or make the driver include the corrected implementation.
Useful? React with 👍 / 👎.
| DEFINES += -DQLI_DTYPE_FP8 | ||
| DT_TAG = _fp8 | ||
| endif | ||
| TARGET = $(ELF_HEAD)/$(TESTCASE)$(DT_TAG)_B$(B)_Sq$(Sq)_Skv$(Skv)_g$(g)_Tm$(Tm)_Tk$(Tk).elf |
There was a problem hiding this comment.
When two experiments use the same batch and tensor/tile dimensions but different topk values, both builds resolve to this identical target path even though -DTtopk changes the binary's output shape and behavior. The later build silently overwrites the earlier ELF, so downstream address-fixing, dumping, or verification can run a binary whose TopK configuration cannot be determined from its advertised filename; add topk to the target name as is already done for the other configuration dimensions.
Useful? React with 👍 / 👎.
| clean: | ||
| @find $(OBJ_ROOT) -type f -name "*.o" -exec rm -rf {} \; |
There was a problem hiding this comment.
Every normal QLI build depends on the phony clean target from Makefile.common, and this replacement recipe deletes every .o beneath the shared one-level-architecture output tree rather than only QLI objects. Building this testcase therefore destroys cached objects for unrelated kernel tests and can also remove objects being produced by a concurrent build, leading to unnecessary full rebuilds or nondeterministic link failures; scope the deletion to this testcase's object directory.
Useful? React with 👍 / 👎.
…hpp) New qli_pto_opt_dynamic.hpp: runtime Sq/Skv/topK (no template shape params) + multi-PE interface (get_thread_idx + stride loop). - Single PE (NPE=1, FP8 direct TMATMUL): verified on all key cases (Sq=4/64, Skv=128/2048/2080/8192, cosine=1.0 set=100% nan=0) - Multi PE (NPE>1): standard cooperative path per matmul_multithread pattern - FP32 conversion (TCVT, FP32 NORM required by v0.58 cooperative TMATMUL), D=128 split into two [64,32] FP32=8KB halves (Shared tile 8KB limit), TMOV_L2S_PUBLISH + TMATMUL/TMATMUL_ACC. Per-PE hist/prefix scratch (256+32 uint32) to avoid histogram races. Numeric verification NOT passing under gfrun (only token 3 partially correct); documented as known limitation pending upstream cooperative TMATMUL support. Sq must be multiple of numPEs. - scores row stride is paddedSkv = ceil(Skv/2048)*2048 (full-chunk reads; tail padding zeroed in key_scratch as key=0 never selected) - Driver qli_check_opt_dynamic.cpp + Makefile entry (NPE param)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 364ef93788
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ```bash | ||
| # 与 Makefile 维度一致 | ||
| export SQ=4 SKV=8192 TOPK=512 G=64 TM=16 TK=32 | ||
| OUTDIR=../../../../../compare/qli_fp8_B1_Sq${SQ}_Skv${SKV}_g${G}_Tm${TM}_Tk${TK} |
There was a problem hiding this comment.
Fresh evidence after the earlier path-replacement reports is that, when these instructions are followed from the repository root, $OUTDIR is expanded only after the cd test/kernel/qli/src, so five .. components resolve it to benchmark/compare/...; the assembly path that Step 2 attempts to produce remains under benchmark/one-level-arch/compare/.... Thus even after correcting the already-reported sed typos, Step 1 generates the blobs in a different directory and Step 3 cannot find them; use four .. components here or make the replacement target the generated absolute directory.
Useful? React with 👍 / 👎.
| uint32_t* hist_scratch = key_scratch + (uint64_t)Sq * paddedSkv | ||
| + (uint64_t)tid * 288; |
There was a problem hiding this comment.
When numPEs > 1, each PE selects a distinct 288-word region using tid, so the required trailing workspace is numPEs * 1152 bytes. The public contract above this function promises only one 1152-byte hist/prefix region; a caller allocating exactly that documented size will have every PE after PE 0 write beyond the buffer during TopK. Include the PE multiplier in the workspace contract and allocation requirement.
Useful? React with 👍 / 👎.
| q + i * Sq * 64 * 128, k, | ||
| w + i * Sq * 64, scale_q + i * Sq * 64, scale_k, |
There was a problem hiding this comment.
When the dynamic target is run with B > 1 after the already-reported Batch/B macro mismatch is corrected (or when compiled with -DB directly), Q, W, and scale_q advance to the current batch but K and scale_k always remain at batch 0. This differs from the static driver, which advances all five inputs, and produces incorrect scores for later batches containing distinct K/scales; offset these pointers by i * Skv * 128 and i * Skv, respectively.
Useful? React with 👍 / 👎.
| | concat | 4 | ✓ | gather/scatter | | ||
| | control | 1 | △ | pure tile-op; run gfsim with `-s core.singleTierMode=true`; `.data` via `gen_data.py` | | ||
| | sort | 1 | △ | topk | | ||
| | qli | 4 | ✓ | radix-select TopK (qli_check_opt); set-match verified; see [`qli/README.md`](qli/README.md) | |
There was a problem hiding this comment.
A repo-wide search finds no QLI invocation in benchmark/one-level-arch/compile_all.sh, whose compile_operator calls explicitly enumerate every suite rather than discovering compile.all files. Consequently the newly listed QLI suite and its four compile configurations are skipped by the repository's full-compilation command, so aggregate runs can report completion without compiling any of this new kernel code; add the QLI suite to that enumeration.
Useful? React with 👍 / 👎.
- gen_cases.py: hist dst array now M*256 (was M*N) to match THISTOGRAM output shape [src.ValidRow, 256] UINT32 - vector_bench.hpp: added bench_hist_u32 with dst tile Tile<Vec,uint32_t,M,256> - Regenerated thistogram_i16/i32_16x16.cpp to use bench_hist_u32 - gen_cases.py hist kind now emits uint32_t ch[M*256] dst buffer
- qli_check_data.s: updated QLI test data addresses - qli_check_opt.cpp: updated QLI test driver addresses - include/: symlinks to tileop-api headers for __cpu_sim__ build
1. Function signature: template_asm.hpp:224 2. Idx shape constraints: AccumulateBlockInfo.cpp:201-207 3. Assembly template: template_asm.hpp:226-233 4. Related histogram APIs: template_asm.hpp:6711-6971 5. Non-qli reference code: thistogram_i32_16x16.cpp:10
Migrate qli kernels to the PTO v0.58.4 tile interface (ops-20260828
toolchain: llvm adcb8794 + TileOP-API f94bc12 + gfrun d8903938):
- Step1-6 matmul: TileLeft/TileRight/Vec-acc + TLOAD/TSTORE ->
CubeTileM16/CubeTileN8/CubeAccumulatorM16 + TLOAD_CUBE/TSTORE_CUBE,
with a GM round-trip bridge (TSTORE_CUBE -> TLOAD) back to Vec tiles
for the ReLU/W/TCOLSUM chain (same pattern as fa_2d_unroll_gmma)
- K operand: stored as transposed [D, Skv] row-major (CUBE_N8 contract);
dynamic variant uses per-kTk-block contiguous layout with per-block
iterators (avoids MaxSkv template-stride mismatch)
- W*scale_q: pre-broadcast to [kTm, kTk] GM by the driver, applied with
plain TMUL (v0.58.4 row-expansion validator requires a physical
one-column broadcast source, incompatible with the 128B tile floor)
- TopK: TCMP tile-tile replaced by TCMPS scalar compares + TSUB diff
(TCMP tile-tile asm emits cmode mnemonics the current LLVM matcher
rejects; THISTOGRAM B.DATR similarly fixed in the local TileOP header)
- Multi-PE dynamic: Q/K per-PE local CUBE tiles (Shared-B rendezvous
semantics not realized under gfrun independent-main model; documented
as known limitation)
Verified on ops-20260828 env (single PE, all cosine=1.0 set=100%):
simple: Sq4/Skv128/8, Sq4/Skv2048/512, Sq4/Skv8192/512,
Sq4/Skv2080/512(tail), Sq64/Skv128/128
dynamic NPE=1: Sq4/Skv128/8 (scores + TopK)
dynamic NPE=4: Step1-6 scores all-correct on 4 tokens; concurrent
TopK index extraction races (only last-finishing PE's TopK intact)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ee5a75c4d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (int d = 0; d < 128; d++) | ||
| for (int c = 0; c < 32; c++) | ||
| ktt[j * 128 * 32 + d * 32 + c] = k[(j * 32 + c) * 128 + d]; | ||
| static float tmp16[16 * 32]; |
There was a problem hiding this comment.
When the advertised dynamic target is built with NPE > 1, qli_pto_dynamic writes each PE's CUBE result to temp_gm + tid * 16 * 32, but this driver allocates only one 16×32 slot. PE 1 and above therefore write beyond tmp16, corrupting adjacent storage and invalidating the multi-PE experiment; size this buffer by numPEs or provide separate per-PE storage.
Useful? React with 👍 / 👎.
| void bench_hist_u32(uint32_t *c, D *a, D *idx, int byteId, auto op) { | ||
| iter_t<D, M, N> gA(a), gIdx(idx); | ||
| auto gA0 = gA(0, 0), gI0 = gIdx(0, 0); | ||
| using dst_tile_t = Tile<Location::Vec, uint32_t, M, 256, BLayout::RowMajor>; |
There was a problem hiding this comment.
For both generated M=16 THISTOGRAM cases, this type creates a uint32_t[16,256] tile, which is 16KB even though the target's active tile limit is 8KB. The cases listed in microbenchmark/vector/compile.all will therefore be rejected or misencoded instead of testing THISTOGRAM; split the rows across multiple destination tiles or reduce the benchmark's M to at most 8.
Useful? React with 👍 / 👎.
| static float* s_default_temp = nullptr; | ||
| if (temp_gm == nullptr) { | ||
| if (s_default_temp == nullptr) { | ||
| s_default_temp = scores_ptr + (uint64_t)Sq * Skv + 2048; |
There was a problem hiding this comment.
When a caller omits temp_gm and allocates only the documented score output followed by kTm*kTk floats of scratch, this offset silently skips another 2048 floats (8192 bytes), so the subsequent CUBE store writes beyond that allocation. Either place the default temporary directly after the scores or explicitly require the additional 8192-byte gap in the public buffer contract.
Useful? React with 👍 / 👎.
| TKey<CK, CKV> bits; TLOAD(bits, gs); | ||
| TKey<CK, CKV> sign; TANDS(sign, bits, 0x80000000u); | ||
| TKey<CK, CKV> neg; TNOT(neg, bits); | ||
| TKey<CK, CKV> pos; TORS(pos, bits, 0x80000000u); |
There was a problem hiding this comment.
When the recommended simple implementation receives the documented canonical NaN 0x7FC00000, this positive-value branch maps it to 0xFFC00000, which ranks above even positive infinity and is therefore selected into TopK. This contradicts the committed status report's promise that NaNs map to zero and are excluded; detect NaN exponent/mantissa patterns before applying the sortable-key transform, as the full implementation does.
Useful? React with 👍 / 👎.
| fill_seq(a, 4096); fill_seq(b, 4096); fill_seq(d, 4096); zero(c, 4096); | ||
| BENCHSTART; | ||
| bench_hist<int32_t,M,N>(c,a,b,0,[](auto& dst,auto& s,auto& idx,auto b){ THISTOGRAM(dst,s,idx,b); }); | ||
| uint32_t ch[4096]; zero(ch, 4096); |
There was a problem hiding this comment.
In both committed THISTOGRAM microbenchmarks, the timed region now includes zeroing 4096 uint32_t elements before executing the single instruction under test. That scalar initialization can dominate the reported latency and makes these results incomparable with the other vector microbenchmarks, whose setup occurs before BENCHSTART; move the declaration and zero call outside the measured region and update the generator likewise.
Useful? React with 👍 / 👎.
| @@ -0,0 +1 @@ | |||
| /home/z00947698/superScalar/linx-toolchain-build/output/linx_blockisa_llvm_musl/lib/clang/15.0.4/include/tileop-api/common No newline at end of file | |||
There was a problem hiding this comment.
When COMPILER_DIR selects any toolchain other than this developer's /home/z00947698/.../linx-toolchain-build install, -I$(ROOT)/include still resolves common through this absolute symlink whenever that path exists, silently mixing headers from one TileOP-API version with a different compiler; on other machines the committed link is simply dangling. This repository explicitly supports arbitrary toolchain install paths, so derive these headers from COMPILER_DIR or remove the machine-specific links.
Useful? React with 👍 / 👎.
| float* temp_gm = nullptr) // [kTm, kTk] CUBE->Vec 桥接临时区 | ||
| { | ||
| constexpr int Qb = Sq; | ||
| constexpr int Kb = Skv / kTk; |
There was a problem hiding this comment.
When Skv is not divisible by kTk—for example the documented Skv=2056, kTk=32 case—this floor division processes only the first 2048 columns, while qli_topk_radix still consumes all 2056 score positions. The unwritten tail can therefore contribute arbitrary indices and corrupt experiment results; add the documented Skv % kTk == 0 static assertion here or implement a tail block.
Useful? React with 👍 / 👎.
1 participant
Summary
Add the QLI (Quant Lightning Indexer) radix-select TopK demo to
one-level-arch.QLI is the preprocessing operator of SparseFlashAttention (SFA): it selects the
topK most important K/V token indices. This PR adds:
kernels/qli/qli_pto_opt.hpp— multi-round MSD radix-select TopK(
qli_topk_radix): float score → IEEE-754 monotonic uint32 sortable key →THISTOGRAM Byte3→Byte0 narrowing (with Idx prefix) → tile pop-argmax
extraction (in-tile zeroing, no scalar stores / no reload).
kernels/qli/qli_pto.hpp— baseline Step1-6 + TROWARGMAX TopK.test/kernel/qli/— demo driverqli_check_opt.cpp, golden generatorgen_qli_golden.py(set-match criterion), data embed + address fix tools,Makefile / compile.all / README with full validation flow.
Validated cases (gfrun, set-match)
Skv=2056(non-kTkmultiple) is not used for topK=512 because Step1-6 onlycomputes 2048 columns;
Skv=2080covers the tail-chunk path.Known issues
Implementation-time emulator/toolchain issues (locally fixed or worked around,
all validated) are collected in
qli_radix_issues_found.md(R1–R4).Notes
BRob stall 31.0% → 14.9%, scalar block ratio 92.8% → 43.4%.