Repository navigation
perf(top-k): pass a statically known K by value - #1330
Open
zhonglinnian wants to merge 2 commits into
Open
zhonglinnian wants to merge 2 commits into
zhonglinnian wants to merge 2 commits into
Conversation
wrap_top_k reads K from a device tensor, which costs a device-to-host copy and a stream synchronization on every call. Add wrap_top_k_static, which takes K by value for callers that know it at compile time; every other argument and the result are the same. Both entry points share one launch helper. wrap_top_k now reads K with hipMemcpyAsync(hipMemcpyDefault) on the session stream before synchronizing it, rather than a blocking hipMemcpy on the null stream followed by a sync. The read is ordered after the kernel that produced K on the session stream, and K may live in host or device memory. The mock runtime gets the matching stub. Extracted from AMD-AIG-AI-SOLUTIONS/hip-ir-runtime patches/hip-ep/0010-gguf-host-implementation-Generic-compiler-runtime-ar.patch, the Top-K hunks only. Co-Authored-By: Claude <noreply@anthropic.com>
hip.top_k's results hold K elements along `axis`, so when that extent is static in the result type it is K. Lower such a TopK to wrap_top_k_static with the extent as an i64 argument; only a dynamic extent keeps wrap_top_k and its device-to-host read of `k` plus stream synchronization. ONNX TopK with a constant K -- argmax-style sampling, for one -- takes the static path. The lowering also rejects an out-of-range axis instead of indexing past the shape. Tests: the hip-to-llvm cases cover a static extent with dynamic batch dimensions and a negative axis (K passed by value) and a dynamic extent with static batch dimensions (K still read from its tensor); the e2e model expects the static entry point; a numeric case feeds K as a graph input with a symbolic extent so both entry points are compared against ORT. Co-Authored-By: Claude <noreply@anthropic.com> Made-with: Claude Code
zhonglinnian
requested review from
CaymanYang,
amd-bqiao,
amd-mingw,
asparkhi,
edelaye,
fhanuman,
qianglin-amd,
sambanerxilinxamd and
wcy123
as code owners
October 26, 2026 08:56
|
Thanks for opening a PR! This project follows LLVM's incremental-development and AI-tool-use Before requesting review, please check that:
Reviewers are assigned through |
L2 Accuracy Results (EP vs CPU)
Threshold: 0.01 | Run: 7910 - Commit: |
Binary sizesBaseline: run 40278658372 -
MorphiZen EP Performance Results
EPContext Export Performance
EPContext Import Performance
OGA Benchmark Results
OGA Wheel Smoke (Python benchmark_e2e.py)
VLM Benchmark Results
Run: 7910 - Commit: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A TopK whose K is known at compile time no longer reads K back from the device.
hip.top_know lowers to a newwrap_top_k_static(..., int64_t k, ...)when its result extent alongaxisis static, which skips a device-to-host copy and a stream synchronization per call: a steady ~8 µs per TopK on gfx1151.Related issue or design
None. The runtime entry point is extracted from hip-ir-runtime's hip-ep patch
0010-gguf-host-implementation-...by Yu Xing (@yuxing12), its Top-K hunks only.Why
wrap_top_ktakes K as a device tensor, so every call copies it to the host and synchronizes the stream before launching the kernel. ONNX TopK's results hold exactly K elements alongaxis; when that extent is static in the result type, K is already known and the round trip is pure overhead. ONNX models with a constant K, such as argmax-style sampling, hit this on every inference.What
wrap_top_k_statictakes K by value; both entry points share one launch helper. Mock stub and declaration added.wrap_top_know reads K withhipMemcpyAsync(hipMemcpyDefault)on the session stream before synchronizing it, instead of a blockinghipMemcpyon the null stream. The read is ordered after the kernel that produced K, and K may be in host or device memory.TopKLowering.cpp: when the results' extent alongaxisis static, callwrap_top_k_staticwith that extent as ani64; a dynamic extent keepswrap_top_k. No op or attribute change.axisis now rejected by the lowering instead of indexing past the shape.Performance
Same branch, one ONNX TopK (K = 1, rows = 1) per graph; "static" has K as a constant initializer (
wrap_top_k_static), "runtime" has K as a graph input with a symbolic result extent (wrap_top_k). Mediansession.run()time over 5 interleaved rounds of ~1 s each.Hardware: gfx1151 (Strix Halo), Linux, TheRock 7.13, ORT 1.31.0, Release build. Round-to-round spread ≤ 1.3 µs. Both paths were confirmed with
HIPDNN_EP_DEBUG=1and timed with it off. The runtime model also sizes its outputs dynamically, so part of its extra time may come from that shape handling rather than the call alone.Test plan
check-hip-mlir-lit: 669 tests, 660 passed, 9 unsupported, 0 failed.hip-to-llvm/test_topk.mlir: the static case expectswrap_top_k_static; a dynamic-batch case with a negative axis checks K = 5 is passed by value; a dynamic-extent case keepswrap_top_k.e2e/test_topk_model.mlirexpectswrap_top_k_static.test/numeric/tests/test_topk.pyon gfx1151:test_topk_axis1[largest=1],test_topk_full_kand the newtest_topk_runtime_k(K as a graph input, sowrap_top_k) pass.test_topk_axis1[largest=0]fails, on both paths with identical wrong indices, so it is an existinghip_top_kissue withlargest=0, not this change.pre-commit run --from-ref origin/main --to-ref HEAD: all hooks pass.Notes for reviewers
wrap_top_k_static).largest=0failure above needs its own fix in the kernel.Checklist
AI assistance: extracted, extended, tested and benchmarked with Claude Code; commits carry
Co-Authored-By: Claude, and the follow-upMade-with: Claude Code.🤖 Generated with Claude Code