Skip to content

perf(rocke): prepare saved paged decode bindings once - #13700

Open
tenpercent wants to merge 31 commits into
developfrom
users/tenpercent/rocke/aick2566-decode-overhead
Open

tenpercent wants to merge 31 commits into
developfrom
users/tenpercent/rocke/aick2566-decode-overhead

Conversation

@tenpercent

@tenpercent tenpercent commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Repeated launches from a saved paged decode binding redo scratch allocation and fixed launch preparation. This PR prepares that work once for gfx942/gfx950 non-FP8, LSE-off one-token split-KV attention. Compatible subsequent calls reuse the compiled pipeline, launch geometry, stream wrapper and private scratch, while reading live tensor pointers and scalar overrides and packing fresh kernel arguments on every launch.

LSE-enabled bindings use the public runner on every invocation, preserving output validation, buffer overrides and fresh reducer arguments.

ISSUE ID : AICK-2566

Risk Assessment

Level 4: this changes a performance-sensitive host submission path, and fresh paired measurements found an unresolved gfx942 default-graph regression. The eager benefit is bounded to the measured saved bindings; it does not establish a general speedup. Incompatible calls retain the public runner. Focused tests cover scratch ownership, live overrides, stream/capture fallback and failure recovery. There are no emitter, kernel-signature or native-provider changes.

ASIC Coverage

The optimized path serves gfx942 and gfx950. Fresh merged-head validation completed on both targets: gfx942 eager and default-graph comparisons, gfx950 eager comparisons, and LSE numerical checks with repeated saved-binding launches. All measured CLI arms passed independent numerical checks. The gfx942 default-graph performance regression still needs a fix and repeat measurements. Native-provider and gfx1250 hardware validation are outside this binding change.

Testing Summary

Develop 3eeb4c446556b6717a37866479fae3b581bfed16 was merged into this branch without conflicts and pushed as 0feeb33b12feb4fc5991706443111c25fc7b4226. Both overlapping CMake test registrations survived. The merged binding implementation and shared runtime are unchanged from the previous PR head.

  • Merged-head focused host cohort: 55 passed, covering test_attention_decode_binding.py, dispatch/attention/test_unified_lse_dispatch.py and test_attention_spec_predicate_parity.py.
  • Merged-head attention dispatch cohort: 698 passed, 3,970 subtests passed, 253 skipped. The skips are RDNA numerical cases requiring matching gfx11/gfx12 hardware. This was not a no-skip GPU or native/parity run.
  • Earlier installed Python attention CPU CTest entries passed through selectively staged Python/library/test install rules. Their 15 skips covered unsupported interleave/cache-policy combinations and the unstaged C++ extension. Those checks were not rerun after this develop merge and do not establish a fresh full native package build.
  • Fresh GPU timing: 36 CLI runs passed, all reporting status="ok", with independent FP32 reference checks on BF16 inputs before timing at atol=0.03 and equivalent executable specs across pairs.
  • Fresh LSE numerics passed on gfx942 and gfx950 against independent FP32 references with BF16 inputs and the existing 0.04 tolerance. Each target passed the pinned split-KV case and four launches of one saved binding. Both outputs were poisoned before every saved-binding launch; the checks covered an LSE-buffer override and verified that the unused buffer remained untouched. Private preparation was rejected by the adapter.
  • PR-range pre-commit hooks passed against the merged develop revision. The LSE guard's earlier Torch-free negative control failed for both architectures when only that guard was removed; that negative control was not repeated after this merge.

Host checks used the Python engine and LLVM23. Fresh GPU runs used Python 3.12, the Python engine, LLVM23 emission, ubuntu-24 and rocm-nightly/10.3.0a20261027, with matching Torch/SDK wheels. The existing timing CLI compiled normally in-process; the separate LSE adapter compiled in Torch-free subprocesses. No new broad native/parity or installed replay result is claimed.

Testing Checklist

  • Merge develop, preserve test registrations, push and verify the remote head
  • Merged-head focused host regressions and attention dispatch tests, skips accounted for
  • Earlier selectively staged installed Python CPU checks, identified as historical above
  • Merged-head PR-range hooks
  • Fresh gfx942 paired base/head HIP-event comparisons, eager and default graphs, both shapes, all 24 numeric checks passed
  • Fresh gfx950 paired base/head HIP-event comparisons, both shapes, all 12 numeric checks passed
  • Fresh gfx942/gfx950 LSE numerical confirmation and repeated binding launches
  • gfx942 default-graph overhead regression: fix and repeat paired measurements
  • Historical host wall-clock enqueue metric reproduction
  • Complete PR CI: required checks currently pass; hipDNN superbuilds, compiler/runtime and emulation passed at the merged head, while multi-arch math-libs stages remain queued/running. Math CI Summary reports a skip rather than GPU execution. Review decision remains CHANGES_REQUESTED.

Technical Changes

Preparation happens on the first ordinary eager launch; creating a binding submits no GPU work. Each binding owns its scratch independently, while existing immutable compiled caches can still be shared. Ordinary runtime retention keeps submitted tensors and scratch alive until queued work completes, including when the binding is discarded.

For eligible prepared bindings, None selects Torch's current stream, and explicit integer stream handles remain supported. LSE bindings retain the existing integer-stream interface and default 0. A different device, stream or tensor layout, a foreign or reentrant host-thread call, internal graphs, or external capture uses the public runner. Explicit stream contexts are restored after each call.

The implementation is confined to the attention binding layer, with focused regressions, installed CPU test registration and documentation. Shared attention kernels and the generic HIP runtime are unchanged relative to develop. The change adds no global scratch cache or general concurrency guarantee. Unfenced callers still need their existing completion boundary before reading outputs.

Performance evidence

The saved decode binding case study records historical paired gfx942/gfx950 measurements against the original per-call binding path. Those measurements predate the LSE admission repair and this develop merge. They apply to saved non-FP8, LSE-off bindings at their recorded configurations and are distinct from the fresh comparison below. HIP-event intervals include host pacing and do not establish a device-kernel speedup.

Runnable saved-binding HIP-event comparison

The existing benchmarks.common.attention_combo_sweep command creates one saved binding and repeatedly launches it. Run this from dnn-providers/hip-kernel-provider/rocke in separate checkouts of base 3eeb4c446556b6717a37866479fae3b581bfed16 and merged head 0feeb33b12feb4fc5991706443111c25fc7b4226, on the same gfx950 GPU and with the same release-matched ROCm/Torch environment:

PYTHONPATH=platform/python:library \
ROCKE_BACKEND=python ROCKE_LLVM_FLAVOR=llvm23 HIPDNN_GFX950_3D_GRAPH=0 \
python -m benchmarks.common.attention_combo_sweep \
  --arch gfx950 \
  --run-candidate attention_gfx950_u3d_splitkv_seg128_t1xb \
  --dtype bf16 --batch 1 --heads 32 --kv-heads 4 \
  --head-dim 128 --seqlen-q 1 --seqlen-k 2048 --kv-block-size 16 \
  --warmup 20 --iters 100 --benchmark-iterations 7 --seed 7

This pins B=1/KV=2048, Hq/Hkv=32/4, D128, page-size-16 BF16 split-KV attention. For the other measured shape, use --batch 4 --seqlen-k 4096. For gfx942, change both architecture/candidate identifiers to gfx942 and use HIPDNN_GFX942_3D_GRAPH=0 for the eager diagnostic.

For gfx942's default internal-graph policy, first run unset HIPDNN_GFX942_3D_GRAPH, then omit the HIPDNN_GFX942_3D_GRAPH=0 assignment from the gfx942 command. Keep outer capture disabled. Internal graphs are on by default for the measured gfx942 shapes; gfx950 is eager by default. The eager gfx942 rows are diagnostics, not its ordinary default behavior.

Compare matching rows only when both report status="ok", with equivalent executable specs and geometry. Compute each paired reduction from the row's HIP-event ms statistic as 100 * (1 - head.ms / base.ms). Repeat paired runs on the same GPU with alternating arm order and record revisions and environment. The CLI's independent FP32 attention reference check remains enabled, using quantized BF16 inputs and default absolute tolerance 0.03.

Fresh merged-head results

On October 28, 2026, three paired trials per shape and policy alternated base/head order on one exclusively allocated GPU per target. All 24 gfx942 and 12 gfx950 CLI runs exited successfully and passed their before-timing independent numerical checks. Executable spec hashes, tuning knobs, kernel names and selected paths matched across each pair. The CLI does not add a poisoned post-timing check; the separate repeated LSE checks described above are a different validation lane.

The CLI takes seven HIP-event samples, each with 20 warmups and 100 timed executions, discards the first sample and selects the upper middle of the remaining six. The table reports the median paired reduction over three trials. The range describes those trials, not a confidence interval. A negative reduction means the head took longer than develop.

Target B / KV Saved-binding policy Median HIP-event interval reduction Paired-trial range
gfx942 1 / 2048 Eager diagnostic, graphs disabled 58.0% 57.4% to 58.1%
gfx942 4 / 4096 Eager diagnostic, graphs disabled 57.8% 57.3% to 58.2%
gfx942 1 / 2048 Default internal graphs -7.9% -8.7% to -6.8%
gfx942 4 / 4096 Default internal graphs -7.9% -8.1% to -7.8%
gfx950 1 / 2048 Eager, graphs disabled 56.5% 56.2% to 57.5%
gfx950 4 / 4096 Eager, graphs disabled 56.5% 56.0% to 56.7%

The gfx942 default-graph intervals were about 7.9% longer for both shapes, consistently across all three paired trials. This regression is unresolved. Eager savings do not establish a benefit for gfx942's default graph policy. These results apply only to the saved Python binding boundary for the measured synthetic non-FP8, LSE-off configurations, with outer capture disabled. They do not reproduce historical host wall-clock enqueue percentages, rotating maximum-length cases or callers that rebind every invocation.

Provenance: gfx942 Slurm job 68202040 on defq and gfx950 job 68202130 on rocke both completed with exit 0:0. Environment: Python 3.12, Python engine, LLVM23, ubuntu-24 + rocm-nightly/10.3.0a20261027, Torch 2.16.0a0+rocm10.3.0a20261027, HIP 7.17.26431, NumPy 2.2.6, and matching rocm-sdk-core, rocm-sdk-libraries and target-specific device wheels version 10.3.0a20261027. Compilation and launch used matching SDK-wheel COMGR/HIP libraries. A fresh gfx950 wheel download could not obtain the pinned device wheel, so that job used a task-local copy of a previously verified environment on the same target; package versions, runtime mappings and device operations were freshly verified. The source and CLI were the pinned merged revisions. Raw source-tar SHA-256 values: cf2337fd6eafb90e4887cedf8dad225438631f3075325029acec84f0c8184235 (base) and 809a1c29edf8cfe8b061b18d2275c00cf0d1a65e5422213639647ddbf437a7b2 (head). Private raw records retain exact commands, adapter hashes and numerical summaries.

Open follow-up plan

The requested gfx942 default-graph ratio is now measured, but it revealed a regression. In _bind_paged_decode_launch, calls resolve the stream, check thread/owner state and query capture before checking the pure graph policy and delegating to the public runner. Those additional wrapper probes are a candidate source of overhead; the exact cause and remedy have not been proven.

The focused follow-up is to measure the wrapper cost, test an early graph-policy delegation that avoids eager-only probes, and extend the existing graph-mode regression tests. It must preserve current/zero/explicit stream semantics, policy changes after binding, external capture, LSE exclusion and private-owner safety. Then repeat the same default-graph pairs and numerics, and check that eager behavior remains correct. No runtime fix or new repository benchmark is included in this evidence update.

@github-actions github-actions Bot added documentation project: hip-kernel-provider rocke-lib: attention Touches rocKE library attention/FMHA kernels rocke: platform Touches rocKE platform labels Oct 15, 2026
@tenpercent
tenpercent requested a balanced review from Copilot October 15, 2026 02:16
@therock-pr-bot

therock-pr-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

gfx1250 rebinding encounters a scope-only restriction, and the shared graph-runtime changes lack current validation for that target.

2 open findings
What changed in this PR

Reduces repeated split-KV decode host overhead through fixed-binding replay scopes without changing kernel emission.

Changes:

  • Adds scoped replay and rebinding with explicit stream and resource ownership.
  • Optimizes ordinary stream setup and graph-binding checks.
  • Updates benchmarks, lifetime tests and usage documentation.

Paths below are relative to dnn-providers/hip-kernel-provider/rocke/.

File Description
platform/​dsl_docs/​optimization/​optimization_runbook.md Adds host-overhead diagnosis guidance.
library/​tests/​test_strided_kv_decode_numeric.py Covers replay updates and storage rebinding numerically.
library/​tests/​test_attention_workspace.py Tests redundant-context avoidance.
library/​tests/​test_attention_3d_replay_scope.py Tests scope ownership, rebinding and cleanup.
library/​tests/​test_attention_3d_replay_bindings.py Tests binding checks and graph-cache lifetime.
library/​kernels/​common/​attention_unified.py Implements scoped replay and runtime optimizations.
library/​builders/​gfx950/​attention/​README.md Links the host-overhead case study.
library/​builders/​gfx950/​attention/​CASE_STUDY_DECODE_HOST_OVERHEAD.md Documents comparison methodology and results.
library/​benchmarks/​gfx950/​attention/​decode/​benchmark_decode_live.py Times the scoped public callable.
library/​benchmarks/​gfx942/​attention/​decode/​benchmark_decode_live.py Times the scoped public callable.
docs/​strided-kv-decode.md Documents replay, rebinding and completion contracts.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@therock-pr-bot

therock-pr-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

Keep ordinary gfx1250 rebinding independent of scope-only admission and retain replacement bindings until completion. Confine context and resident-key shortcuts to gfx942/gfx950 while preserving prior gfx1250 graph behavior. Add host and numerical regression coverage for AICK-2566.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Asynchronous GPU ownership and drain-failure recovery warrant final human review, particularly with the reported develop conflicts.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

@tenpercent
tenpercent marked this pull request as ready for review October 15, 2026 14:55
@tenpercent
tenpercent requested review from a team as code owners October 15, 2026 14:55
@tenpercent tenpercent changed the title perf(rocke): recover split-KV decode timing with scoped replay fix(rocke): remove repeated split-KV decode submission overhead Oct 15, 2026
@tenpercent tenpercent changed the title fix(rocke): remove repeated split-KV decode submission overhead perf(rocke): prepare saved paged decode bindings once Oct 24, 2026
@tenpercent
tenpercent requested review from a team, AnaghaRaoAMD, AviralGoelAMD and cgmillette and a balanced review from Copilot and removed request for a team, AviralGoelAMD and cgmillette October 24, 2026 03:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Asynchronous scratch reuse and stream/capture lifecycle handling warrant final human review of the implementation and reported hardware validation.

1 open issue
1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reusable GPU scratch across stream, capture and asynchronous-lifetime boundaries warrants final human review of the lifecycle guarantees and reported hardware validation.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #13700 — prepare saved paged decode bindings once — 🔴 Follow-up: blocking — the narrowed change is sound on its own base, but merged with today's develop it breaks #13955's LSE binding: a CPU-lane test fails in every CI shard, and the second call of an LSE-emitting binding raises

Prior item Status
Copilot (10-21), our 🟠: the cross-device retention key 🟢 N/A now: the generic runtime changes were withdrawn, and every runtime file is identical to the base (ran). The author records the inherited FIFO device-isolation issue as separate runtime work
Our 🟠 (10-21): no gfx942 hardware run 🟢 ADDRESSED per the description: the gfx942 and gfx950 runtime numerical suites pass (64 each), lifecycle checks pass (9 each), and numerics pass before and after every timed arm (48 each). We have no gfx942 / gfx950 here to repeat them (read)
Copilot (10-21), our 🟡: single-host-thread contract 🟢 Moot: foreign-thread and reentrant calls now fall back to the public runner, and CI runs test_foreign_thread_and_reentrant_pair_get_private_scratch (read)
Other reviewers since 10-21 (AnaghaRaoAMD, AviralGoelAMD, CongMa13, Copilot ×3 rounds) 🟢 All 28 threads are resolved; most were about code that has since been withdrawn (read)
Bots / CI 🟢 Green at 0db55f1e (10-24), with the new test file passing 13 in all four shards (read). That run predates #13955's merge
Our checks 🟢 The PR now changes 6 files and one production file, bindings.py. The merge with today's develop is textually clean (152 behind) (ran). The new binding test passes 13 on the merge (ran). The dispatch/attention host suites pass 647 on the merge, except for the 2 failures in N1 (ran). rule_scan is clean (ran)
# Sev file:line Finding
N1 🔴 library/dispatch/attention/bindings.py (_bind_paged_decode_launch) Semantic conflict with develop's #13955 (LSE output, merged 10-24).
• CI: rocke_library_attention_pytest collects all of tests/dispatch/attention in every shard, without torch. On the merge, test_unified_lse_dispatch.py::test_binding_forwards_the_lse_buffer[gfx942] and [gfx950] fail with ModuleNotFoundError: No module named 'torch'. The decode fast path imports torch before it decides whether to take the prepared path, whereas develop's binding calls the (patched) runner directly; on develop both pass (ran).
• Behavior: the warm path builds reduce from reduce_defaults plus output_ptr and seq_lens_ptr, never lse_ptr. An emit_lse spec's reduce kernel takes lse_ptr, and the packer raises KeyError: missing kernel arg for an absent argument. So the second and later calls of an LSE-emitting saved decode binding fail (read).
→ Rebase, then either route emit_lse bindings to the public runner as use_fp8 ones are, or carry lse through the prepared path (its layout check and the reduce arguments). Keep the torch import behind the cheap checks.

Utility: 🟢 Positive — the narrowed design keeps all reuse inside one saved binding, with private scratch and the ordinary retention path, and roughly halves host enqueue time on gfx942 / gfx950 by the description's ratios. It needs the LSE path reconciled before landing.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were a file-level comparison with the base and the threads against the code. They also included the new binding test with torch on a merge with today's develop, the dispatch/attention host suites on the merge and on develop (which found N1), a read of the 3D launch and kernarg packing, and the CI logs at head. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

Merge develop and exclude LSE-enabled problems from private decode preparation. Preserve repeated LSE buffer and stream forwarding with a regression for both affected architectures.
@CongMa13

Copy link
Copy Markdown
Contributor

I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:

If you have already fixed these, please reply on each comment to say so and resolve the threads.

@tenpercent

tenpercent commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Following up on the October 26 LSE finding, verified at merged head 0feeb33b12feb4fc5991706443111c25fc7b4226 against develop 3eeb4c446556b6717a37866479fae3b581bfed16:

N1 remains addressed by bc79b92167748cc4614144260a064dd470ef1182. The eligibility guard excludes emit_lse before creating the prepared helper. LSE bindings use the public runner on every invocation, preserving output validation, fresh reducer arguments and live LSE-buffer/integer-stream overrides. The warm prepared path without lse_ptr is unreachable for these bindings.

The merged-head focused binding/LSE/predicate cohort passed 55 host tests. The earlier Torch-free cohort passed 83 tests, and removing only the LSE guard failed the regression for both architectures; those earlier checks are identified separately in the description.

Fresh merged-head numerical confirmation now passed on both gfx942 and gfx950. Each target passed the existing pinned split-KV case and four launches of one saved binding, with BF16 inputs and the existing FP32-reference tolerance. Both outputs were poisoned before each saved-binding launch, the LSE-buffer override was exercised, and the unused buffer remained untouched. The adapter rejected private preparation. The PR description records the source/environment provenance and validation scope. gfx942 LSE confirmation is complete.

Assessment and reply prepared with Codex.

@tenpercent

tenpercent commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Following up on the October 23 review summary, at merged head 0feeb33b12feb4fc5991706443111c25fc7b4226:

  • Validation accounting: the description now separates the merged-head cohorts (55 focused host tests; 698 attention dispatch tests and 3,970 subtests, with 253 matching-hardware skips) from earlier installed Python checks. The unstaged C++ extension and historical installed scope remain explicit. The unchanged strided parity suite has not been newly completed with a built engine and is not presented as proof of this binding optimization.
  • Scope and thread handling: the global eager cache, generic-runtime coordinator and scoped replay/rebind work were withdrawn. Only the creating thread reuses a binding's private scratch; foreign-thread/reentrant calls fall back to the public runner. This establishes local scratch separation, not a general runtime concurrency guarantee.
  • Measurement coverage: the existing benchmarks.common.attention_combo_sweep command has now been run against develop 3eeb4c446556b6717a37866479fae3b581bfed16 and merged head for both recorded shapes, on gfx942 with eager/default graphs and on gfx950 with eager launches. Three alternating paired trials per shape/policy produced 36 successful CLI runs; all passed independent FP32-reference checks and matched executable specs. The PR description contains the runnable command, verified relative results, exact revisions and provenance.

The gfx942 default-graph measurement request is complete, but it revealed a consistent regression. The description keeps its fix/revalidation open and records a focused plan to measure wrapper overhead, test early graph-policy delegation while preserving stream/capture/live-policy contracts, extend existing tests and repeat the same pairs. This plan is not an implemented or validated remedy.

The historical host wall-clock enqueue percentages remain a separate open reproduction request because the existing CLI measures HIP-event intervals. Per-invocation rebinding and rotating maximum-length cases remain outside the measured scope. Fresh LSE numerics passed on both targets. No new repository benchmark or performance fix was introduced in this evidence update.

Assessment and reply prepared with Codex.

@tenpercent

tenpercent commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Following up on the October 24 review summary, verified at merged head 0feeb33b12feb4fc5991706443111c25fc7b4226 against develop 3eeb4c446556b6717a37866479fae3b581bfed16:

The runtime-coordinator and eager-cache-clear findings remain addressed by withdrawal. hip_module.py, launcher.py and the shared attention runner are identical to develop; the remaining binding owns private scratch and adds no process-wide launch/synchronization lock or global clearing API.

The requested gfx942 develop-to-head comparison with default internal graphs is now complete. The existing benchmarks.common.attention_combo_sweep command ran unchanged for both recorded shapes, with HIPDNN_GFX942_3D_GRAPH unset and no outer capture. Three alternating paired trials per shape passed all 12 independent numerical checks and matched executable specs. Separate eager comparisons passed on gfx942 and gfx950. The verified relative results, runnable commands and provenance are in the PR description.

The default-graph measurements revealed a consistent regression, so the performance issue remains open. The binding wrapper resolves the stream, checks thread/owner state and queries capture before checking graph policy and delegating. Those additional probes are a candidate source of overhead, not a proven root cause.

The follow-up plan is to measure wrapper cost, test early graph-policy delegation that avoids eager-only probes, extend existing graph-mode tests and repeat the same default-graph pairs and numerics. The change must preserve current/zero/explicit streams, policy changes after binding, capture, LSE exclusion and private-owner safety. No remedy has been implemented or validated yet. The historical host wall-clock enqueue metric is also still a separate open reproduction request.

Assessment and reply prepared with Codex.

@tenpercent

tenpercent commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Following up on the latest reminder: the three linked summaries have separate author replies, now updated for merged head 0feeb33b12feb4fc5991706443111c25fc7b4226 and develop 3eeb4c446556b6717a37866479fae3b581bfed16.

The LSE semantic conflict remains fixed, and fresh repeated-binding LSE numerics passed on both gfx942 and gfx950. The coordinator/cache-clear findings concern withdrawn code. The description separates merged-head host counts and matching-hardware skips from earlier installed Python validation.

All 28 inline threads are resolved and have author replies. Fresh paired base/head timing completed on gfx942 (eager and default graphs) and gfx950 (eager), with all 36 CLI numerical checks passing and equivalent executable specs. The PR description records verified relative results, current revisions, environment and the existing runnable command.

The gfx942 default-graph comparison revealed a consistent regression. Its fix and repeat measurements remain open, with a focused wrapper-overhead plan in the description. Historical host wall-clock enqueue reproduction also remains open. Resolving the older inline threads does not mean those follow-ups are complete. No new repository benchmark or performance fix was added in this evidence update.

Assessment and reply prepared with Codex.

@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #13700 — prepare saved paged decode bindings once (tenpercent, AICK-2566) — 🟠 Follow-up: LSE fixed, gfx942 default slower — bc79b921 keeps LSE bindings on the public runner and their test now passes on the merge with develop and in every CI shard, but the description's own paired runs show gfx942's default internal-graph path slower than develop, and that fix is still open

Prior item Status
Inline threads (10-15…10-24), all 28: Copilot ×9, AviralGoelAMD ×15, AnaghaRaoAMD ×4, incl. the items behind our 10-21 🟠/🟡 (cross-device retention key, stream-handle collisions, drain races, single-host-thread contract) 🟢 N/A or FIXED — all resolved with author replies. Most target withdrawn code: the net diff now touches only the binding, its two tests, the case study, two README links and CMake, so hip_module.py, launcher.py and attention_unified.py match develop (ran: git diff). Foreign-thread and reentrant calls take the public runner (bindings.py:403-406). The doc and test asks are met by the new case study (no PR or job IDs, one sampling recipe) and by test_warm_binding_reuses_private_scratch_and_context, which also enters through the candidate's bound_torch (ran)
Our 10-19 review: CHANGES_REQUESTED at 931a48eb (timing-test stubs; the PR's tests in no CI lane) 🟡 Stale, still standing — it is still the PR's review decision. Its reasons look addressed: the timing tests left the PR with the redesign, and the new test_attention_decode_binding.py is in the CPU attention lane (platform/CMakeLists.txt:637) and passes 13 in every CI shard. No approvals yet; reviews are re-requested from us, AnaghaRaoAMD, AviralGoelAMD and the rocke-lib / rocke-platform teams (ran: gh)
Our 10-21 🟠: no gfx942 hardware run 🟢 ADDRESSED — the description reports fresh merged-head gfx942 runs: eager and default-graph pairs with numerics checks before timing, and repeated-binding LSE numerics. We have no gfx942 here to repeat them (read)
AviralGoelAMD (10-23) review: skip labels and counts disagree; no reproducible develop→head ratio; no gfx942, per-call-rebinding or graph-default measurement; silent cross-thread misuse 🟢 Mostly ADDRESSED — the description separates merged-head counts from earlier ones, gives a runnable attention_combo_sweep command with paired ratios, and measures gfx942; cross-thread calls fall back (row 1). 🟡 DEFERRED, acceptable: per-call rebinding and the historical host-enqueue metric ("outside the measured scope"), since the claim is scoped to saved bindings. The graph-default result is the 🟠 row below (read)
AnaghaRaoAMD (10-24): add the gfx942 develop→head ratio with internal graphs at their default, which is on for these decode shapes 🟠 OPEN, valid — measured on 10-28. The description reports gfx942's default-graph path slower than develop for both shapes, in every paired trial, and leaves the fix as an unchecked checklist item with a plan. By reading, each call resolves the stream, checks thread and lock, imports torch and queries capture (bindings.py:402-410) before the graph-policy check at :411 passes it to the runner, which resolves the stream again (attention_unified.py:4415). The committed case study still shows only graphs-disabled rows, measured at branch commit b106aeda → check the graph policy first, as planned, re-measure, and put the default-graph result in the case study (read)
Our 10-26 N1 🔴: merged with develop, #13955's LSE binding broke: a CPU-lane test failed without torch, and warm LSE calls lacked lse_ptr 🟢 FIXED bc79b921 — the decode gate now requires not problem.emit_lse (bindings.py:585), so LSE bindings never build the warm path or import torch. Every launch goes to the public runner, with live lse and stream overrides. test_binding_forwards_the_lse_buffer (three launches; preparation patched to fail) passes on the merge with develop f22bed43, 31 of 31 in its file (ran), and in all four CI shards at head (read)
CongMa13 (10-28): our 10-26 comment and the 10-23 / 10-24 review summaries had no author reply 🟢 ADDRESSED — the author answered all three on 10-28 (read)
Bots / CI 🟢 At 0feeb33b: the policy bot passes; 45 checks passed, 51 skipped, none failed. Pre-commit, clang-tidy, the Multi-Arch and Component CI summaries, Multi-Arch ASAN and both hipDNN superbuilds pass; Math CI reports success but says it skipped. Multi-Arch CI tested gfx94X (Linux) and gfx110X (Windows): all four Test hipkernelprovider shards pass, each with test_unified_lse_dispatch.py 31 passed (both test_binding_forwards_the_lse_buffer cases) and test_attention_decode_binding.py 13 passed, without torch. gfx950 and gfx125X were built, but their test jobs were skipped. GitHub: MERGEABLE, merge state BLOCKED by the review decision (read)
Our checks 🟢
• Merge: clean with develop f22bed43 (22 behind, 31 ahead) (ran). Since the PR's base, develop changed only 2D sliding-window routing (#14150) in the attention runtime; the 3D split-KV launch the warm path reuses is untouched (ran: git diff).
• Interdiff since 0db55f1e: the PR's own change sits in merge bc79b921: the emit_lse gate and its docstring, the stronger LSE binding test, and case-study text. The develop merges 7b37892b and 0feeb33b add no manual change (ran: git show --remerge-diff).
• Build: the native build and 33 ctests are OK (1 skipped) (ran).
• Goldens: the differential golden (ll at llvm20 / 22 / 23 / 24, and canonical) has no unblessed change and nothing missing; the PR changes no emitter code (ran).
• Tests: the PR's two test files pass 44 on the merge; test_unified_lse_dispatch.py alone, 31 passed (ran).
• Rules 1–3 clean, and the case study gives ratios only (ran)
# Sev file:line Finding
N1 🟡 dispatch/attention/bindings.py:441, :477-497 The warm path skips the runner's per-call LSE and bounds handling.
• At head: a stray lse= override on an LSE-off saved binding is silently dropped on warm calls, so the caller's buffer is never written. The cold call, and develop on every call, raise lse requires problem.emit_lse=True (_lse_store.py:363) (ran: a probe on the merge, built on the PR's own test fixture).
• With #13128 (open, same author): bindings.py conflicts in 2 hunks. The LSE test file and CMake auto-merge, and the two PRs edit different tests in that file (ran: git merge-tree). #13128's attention_paged_decode_t32 candidate (gfx942 / gfx950, page size 1 by default) always uses metadata containment and binds through this function. Containment adds a fourth workspace tensor, so the three-name unpack at :441 would raise on every launch, after its work is queued. The warm dicts also lack the containment arguments and per-call bounds. The PR's tests fake the pipeline, so CI would not notice (read).
→ Route a non-None lse to the public runner now. Whichever PR lands second sends contained specs to the runner, or ports those arguments.

Utility: 🟡 Mixed — faster eager saved bindings on gfx950; gfx942's default graph path is slower.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

Follow-up review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. The checks were an interdiff of the PR's own change since the head we reviewed (0db55f1e); a merge with develop f22bed43 with the native build and ctests, the differential goldens, the PR's two test files, and a probe of the warm path built on the PR's own test fixture; a merge simulation with #13128; and the threads, reviews and CI logs at head. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

@CongMa13

CongMa13 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:

If you have already fixed these, please reply on each comment to say so and resolve the threads.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants