test(rocke): add a launch-path kernarg-packing A/B profiler - #12168
Draft
AviralGoelAMD wants to merge 1 commit into
Draft
AviralGoelAMD wants to merge 1 commit into
AviralGoelAMD wants to merge 1 commit into
Conversation
ISSUE ID : AICK-1513 Splits the profiler out of the quad_perm PR so the intrinsic and the kernarg-packer change can land without waiting on a measurement-validity issue in this tool. The script answers what a packing-only microbenchmark cannot: packing got faster, but what FRACTION of the complete Python launch path was it? The denominator is one call to KernelLauncher.__call__ on its async fence=False branch. Two arms in one process -- pack_args versus compile_packer -- alternated A/B/A'/B' so drift hits both equally, with the real library launcher in the loop for both arms; the arm is installed on launcher._packer and then verified from the code path, not assumed. Guards that make the output trustworthy: fence=False is ASSERTED via _resolved_fence rather than assumed, since a fence puts a device sync inside the timed region and collapses the share toward zero; the noise floor is the same-arm repeat spread, and a delta is called real only if it exceeds it; a back-pressure gate marks a run invalid when enqueue is blocking, because async enqueue is only a host clock while the host outruns the device. Known issue, documented in the code and not fixed here: the chunk-size-sensitivity check is a RATIO, so a chunk size large enough to saturate BOTH windows returns a ratio near 1.0 and the gate accepts a fully back-pressured run. Closing it needs an absolute host-floor reference rather than a second window. Tests cover the pure helpers with no GPU and no rocke import, including the negative cases: an effect smaller than the same-arm drift must be REFUSED, and one stall must not trip the gate.
github-actions
Bot
added
project: hip-kernel-provider
rocke: platform
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
AviralGoelAMD
added a commit
that referenced
this pull request
Sep 16, 2026
#12070) ISSUE ID : AICK-1513 ## What this is PR 1 of a 4-PR stack that splits the GDN gfx950 work (originally one 51-file PR) into reviewable layers. This is the foundation layer: it adds a new crosslane hardware primitive and a launch-path speedup, with **no GDN code** and no dependency on the other three PRs. Stack (bottom → top): **quad_perm (this)** → dispatch.core hoist → GDN decode → GDN prefill. ## What changed **1. `quad_perm` — an intra-quad DPP permute intrinsic, added to both engines.** `quad_perm(data, [p0,p1,p2,p3])` lets lane `4q+i` read `data` from lane `4q+perm[i]` on the VALU (via `v_mov_b32_dpp` / `llvm.amdgcn.update.dpp.i32`), with no LDS crossbar and no `lgkmcnt` wait. Also adds `warp_shuffle_xor_quad`, a fast path for the two xor-masks that stay inside a quad: masks 1 and 2 lower to `quad_perm`, and **any other mask is rejected** — callers needing a wider mask use `warp_shuffle_xor`, which goes through `ds_swizzle`. - Python engine: `core/ir.py` (builder + validation), `core/lower_hip.py`, `core/lower_llvm.py`. - C++ engine (live): `core/ir/ir_flow.cpp`, `core/ir/core_types.cpp` (opcode + purity), `core/lower_hip/lower_hip_mma.cpp`, `core/lower_llvm/crosslane.cpp`, `include/rocke/ir.h`. - Coverage: Python `tests/test_rocke.py`, C++ `tests/core/future_intrinsic_lowering.cpp`, and cross-engine parity `tests/instances/parity/target_intrinsics_emit.{c,py}` (identical IR from both lowerers). **2. Kernarg-packer precompile — a launch-path speedup for every kernel.** `runtime/packing.py` gains `compile_packer(signature)`; `runtime/launcher.py` compiles the packer once at module-load and calls it per launch instead of re-deriving the layout each time. Byte-identical to `pack_args`. What it removes is **kernarg layout reconstruction** — the offset/alignment walk, the per-argument type dispatch, and the format-string assembly. It is *not* a saving on format compilation: CPython's `struct` module already caches recently used format strings, so re-packing the same format is a cache lookup, not a recompile. ## Review round 1 — what changed since the first push Four items were raised by @yraparti and @tenpercent. All four are addressed. **1. Lowerers accepted out-of-range `ctrl` (`bffb5fd911f`).** All four `quad_perm` lowering sites masked the control word with `0xFF` instead of validating it. `ctrl` packs four two-bit lane selectors (`p0 | p1<<2 | p2<<4 | p3<<6`), so `0..255` is the whole legal range — and masking silently rewrote malformed IR into a *different, valid* permute: `256` became `0` (`[0,0,0,0]`, a lane-0 broadcast) and `-1` became `255` (`[3,3,3,3]`). A wrong reduction then computed wrong numbers instead of failing. The builders already validate selectors, but IR reaching a lowerer by another route (deserialized, rewritten by a pass, hand-built) skips them. Now rejected in the Python lowerers (`ValueError`) and the C++ ones (`ROCKE_ERR_VALUE`), with the mask dropped so the check cannot be bypassed. Tests on both sides are **mutation-verified**: with the mask restored, the Python subtests and the four C++ assertions fail. **2. Wave-size semantics undocumented (`cfeec0a1315`, `8cea213942f`).** `quad_perm` makes no wave-size assumption, and both engines now say so: the control word applies within every four-lane group, and four divides both 32 and 64, so a lane never addresses outside its own quad. Wave size changes only the *number* of quads (8 in wave32, 16 in wave64). Deliberately scoped — wave-size-independent is **not** architecture-independent. The op still needs DPP-capable hardware; the useful point is that base-DPP `quad_perm` is available on CDNA where `dpp_xor`'s RDNA-only `row_xmask` is not. Also recorded: the op has no lane targeting (the control is broadcast to every quad, row/bank masks fixed at `15, 15`), so selecting a subset of quads is the caller's job. A first draft of this docstring cited `dpp_xor` as an op whose partner lane can leave the wave; `8cea213942f` corrects that. `dpp_xor` caps `xor_mask` at `1..15` and its partner stays inside a 16-lane row, which also divides both wave sizes. The accurate counterexample is `warp_shuffle_xor` at `lane_xor = 32`. **3. "The dominant Python cost" was unbacked (`786f6c827b0`).** Correct: a packing-only microbenchmark cannot establish a share of launcher overhead. Both sites now describe the mechanism instead of a magnitude -- the packer precomputes the fixed argument layout once per launcher, so a launch does not rebuild the offset table, re-dispatch on argument types, or re-assemble the format string. `struct` already caches recently used format strings, so the saving is that surrounding work rather than the format compile itself. The profiler written to measure the share now lives in its own PR (#12168, draft), and performance claims are deferred until its gate is fixed: its chunk-size-sensitivity check is a ratio, so a chunk size large enough to saturate both comparison windows accepts a back-pressured run. **4. PR description misdescribed `warp_shuffle_xor_quad`.** It said the helper "leaves wider masks on `ds_swizzle`", implying a dispatcher. It is a specialist: masks other than 1 and 2 raise. Corrected in the What-changed section above. ## Scope note (disclosed) Item 2 is functionally independent of item 1. It is kept here rather than split for one concrete reason: its byte-identity test (`test_compile_packer_matches_pack_args_byte_for_byte`) lives in `tests/test_rocke.py`, the same file that holds the `quad_perm` tests — splitting would put two PRs on one test file. Reviewers should weigh the packer's **global blast radius** (it changes the launch path for every kernel), not only its decode benefit. ## Why it's safe - No kernel IR changes — this only adds an opcode and a runtime fast path. - `quad_perm` is validated at the builder (selectors ∈ 0..3, i32-only) **and now at every lowerer** (`ctrl` ∈ 0..255), and is marked pure in both engines. - `llvm.amdgcn.update.dpp.i32` is an already-shipped overload (used by `mov_dpp8`/`ds_swizzle`), not a new intrinsic surface. - The packer is asserted byte-identical to the existing `pack_args`. ## Why it's first in the stack GDN decode's in-quad xor-butterfly reduction lowers to `quad_perm` (3 call sites), so this must land before the decode PR. It touches only `platform/`, so it reviews without any GDN context. ## Verification (run, not planned) Both engines built and exercised locally on this diff: - Python, with the C++ extension importable: `pytest tests/test_rocke.py -k "quad_perm or warp_shuffle_xor_quad or compile_packer"` → **8 passed, 0 skipped** (the cross-engine assertions no longer skip). Full file: **291 passed, 14 skipped, 43 subtests**. - C++ engine: `rocke_future_intrinsic_lowering` → **31 case(s) OK**. - Cross-engine byte-identity: `tools/check_byte_identity.py --only target_intrinsics` → **GREEN, configs=8, bad=0** — Python and C++ emit identical `.ll`. - `test_rocke_ci_static.py` **5 passed**. - Formatters: `black` clean; `clang-format` applied (local binary is v20; repo pre-commit pins v18.1.4, so CI may adjust whitespace). GPU numerical execution of `quad_perm` itself and the full all-family byte-identity gate were **not** run here. ## Stacking / base #11807 has merged, and this PR is now based directly on `develop`.
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
1 participant
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.
ISSUE ID : AICK-1513
Draft. Split out of #12070 at review request so the
quad_permintrinsic and the kernarg-packer precompile can land without waiting on a measurement-validity issue in this tool.What it is
benchmark/perf/examples/profile_launch_overhead.pyanswers a question a packing-only microbenchmark cannot: the precompiled kernarg packer is faster, but what fraction of the complete Python launch path was packing?A share needs a named denominator. Here it is one call to
KernelLauncher.__call__on its async (fence=False) branch — the production hot path — and nothing else.Two arms in one process, alternated
A/B/A'/B'so drift hits both equally:pack_args(signature, values)— the pre-precompile behaviourcompile_packer(signature)The arm is installed on
launcher._packer, so the real library launcher stays in the loop for both arms — nothing is copied into the script that could drift from the library.verify_arms()then confirms from the code path (not from assumption) that each arm holds the object intended and that both produce byte-identical kernargs.Follows the existing
profile_gemm_sweep.py+test_profile_gemm_sweep.pyconvention.Guards
Each exists because its absence produces a confidently wrong number.
fence=Falseis asserted, via_resolved_fence, not assumed. A fence puts a device sync inside the timed region; GPU execution then swamps the host path and packing's share collapses toward zero.valid: falsewhen enqueue is blocking, since async enqueue is only a host clock while the host outruns the device.in_chunk_stepuptimes consecutive segments of one undrained chunk;chunk_size_sensitivitycompares per-launch cost at two chunk sizes.Known issue — why this is a draft
chunk_size_sensitivityis a ratio, so it only detects saturation that the small window escapes. Call it with asmalllarge enough to saturate as well and both windows pay the same per-launch penalty, the ratio returns toward 1.0, and the gate accepts a run that is entirely back-pressured.Deriving
smallfromlarge(currentlylarge // 8) does not guarantee it sits below the queue depth. A ratio cannot express "both windows are slow" — closing this needs an absolute reference, a measured host-only floor, rather than a second window. Documented in the function's docstring; not fixed here.A second, smaller one: min-of-reps cut the gate's false-positive rate on a shared node from 4/110 to 2/110 (ratio IQR 0.140 → 0.020), but did not eliminate it. The survivors are stalls that outlast all five repetitions. The failure is conservative — the gate invalidates a good run, it never blesses a back-pressured one.
Status
Runs end to end on gfx950 (MI355X), correctness-gated against a torch fp32 reference. The gate fires decisively on genuine saturation: at the measured queue knee (~16,000 undrained launches, device 25.2 µs/launch vs host floor 8.4 µs) both checks report 1.31 and 1.79 against a 1.15 tolerance.
Unit tests cover the pure helpers with no GPU and no
rockeimport. The load-bearing cases are the negative ones: an effect smaller than the same-arm drift must be refused, and a single stall must not trip the gate.