Repository navigation
[rocjitsu] Reuse decoded instructions in the CU issue loop - #12632
Conversation
MT19937 initialization repeatedly executes a long scalar and LDS loop. The byte instruction cache avoids fetches, but each issue still decodes, constructs and destroys the same instruction and operand objects. Retain completed non-memory instructions in a bounded, lazily allocated 256-entry cache per CU. Match PC, VMID and all fetched words, including literals. Preserve fetch permission, revocation and byte-cache checks before reuse, bypass reuse while debugging, and clear entries before replacing the decoder. Allocate CU instructions on the heap without installing a decoder pool on the constructing thread. Keep ownership exclusive during issue and asynchronous execution, independently of worker and decoder lifetimes. Exclude memory operations, dynamic state, borrowed fetch-buffer encodings, failed execution and exception unwinding from retention. Document decoded re-execution in Instruction and the execution generator. Cover current registers and EXEC, DPP delegate restoration, literal and VM changes, debugger writes, cache collisions and ownership transfer. Test unrelated allocator hooks and sequential worker lifetimes, and qualify the borrowed VOPD exclusion on RDNA3, RDNA3.5, RDNA4 and CDNA5. On the reviewed base, all 162 selected cache, dispatch, async, matrix, wait-scoreboard and lane checks pass in native, forced-scalar and ASan/UBSan modes. The broad native suite retains only its two known DBT fixture failures. The registered launcher-handoff check and formatting checks pass. Benchmarks use the reviewed cache implementation atop 0510825. Three alternating matched Release pairs against that lane-fixed base reduce median time for the exact gfx942 MT19937 jump kernel with 512 engines from 24.805 to 22.110 seconds at 32 dispatch threads (1.12x). Every raw state word matches the physical gfx1201 reference. The complete exact-nightly 16,384-engine integer API case passes in 311.493 seconds with the original CI topology. Matched IREE and controlled-compiler Gluon selections preserve every case outcome and compiled kernel hash. All 195 IREE cases pass; Gluon retains 129 passes, nine skips and its two baseline TDM gather failures.
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-ISA instruction reuse and allocator ownership affect broad execution and asynchronous lifetime semantics requiring final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Adds bounded decoded-instruction reuse to reduce repeated decode overhead while preserving fetch validation and execution ownership.
Changes:
- Introduces a 256-entry per-CU decoded instruction cache.
- Integrates cache reuse with debugging, async execution, and heap allocation.
- Documents and tests safe instruction re-execution and allocator lifetimes.
| File | Description |
|---|---|
emulation/rocjitsu/tests/instruction_cache_test.cpp |
Adds cache correctness and lifetime tests. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/decoded_instruction_cache.h |
Implements the decoded instruction cache. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/compute_unit.h |
Adds per-CU cache ownership and reset behavior. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/compute_unit.cpp |
Uses cached decodes in the issue loop. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/async_scoreboard.h |
Updates async lifetime documentation. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/instruction.h |
Documents reusable-instruction and allocation contracts. |
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/decoder.h |
Clarifies decoder-pool lifetime requirements. |
emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py |
Documents generated executor reuse requirements. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
amd-eochoalo
left a comment
amd-eochoalo
left a comment
There was a problem hiding this comment.
Ran peanut review with two reviewers (irene and felix). Looks good to them!
sjain-stanford
left a comment
sjain-stanford
left a comment
There was a problem hiding this comment.
Could you extend the performance evidence to cover the allocation change, following emulation/rocjitsu/docs/benchmarking.md?
The three reported MT19937 pairs do not balance run order under that protocol. Please use an even number of alternating pairs and include fresh-process peak RSS plus cycles:u/instructions:u where available, stating any measurement limitation.
The cache retains up to 256 heap instructions per active CU, while each decoder still embeds its 64 KiB arena. A representative large-topology run would establish the actual footprint; object-size estimates alone do not establish RSS.
Please also include a memory-heavy or low-reuse workload at one engine/dispatch thread, plus a representative short-kernel or suite aggregate. When construction and execution share a thread, the old path could use the decoder pool; memory instructions now allocate on the heap on every issue and cannot benefit from retention. The 32-thread MT19937 timing and unchanged IREE/Gluon outcomes do not measure that tradeoff. This requests qualification of a possible regression, rather than asserting one.
We should update the ownership and reuse documentation together so it describes the new issue path consistently. Paths below are relative to emulation/rocjitsu/:
docs/mma-admission.md:46still says ordinary execution decodes every issued instruction. Explain that eligible completed instructions can now be reused, while admission plans remain a separate cache of byte snapshots and decisions. A short addition atdocs/vm-design.md:141can connect this to the authoritative fetch checks, PC/VMID/word key, debugger bypass, and retention exclusions.lib/rocjitsu/src/rocjitsu/vm/amdgpu/async_scoreboard.h:54,matrix_coexecution.h:65in the same directory, anddocs/async-instructions.md:80still tie destruction to the decoder allocator's thread. Issued instructions are heap-backed; the issuer owns and retires queued MMA instructions after helper completion.- Scope the blanket CU heap claim in
lib/rocjitsu/src/rocjitsu/isa/instruction.h:116anddecoder.h:34to issued instructions. Admission lookahead callsDecoder::decode()directly (mma_admission.h:184), so an ambient pool can still serve its short-lived temporaries, which never escape the scan. - Put a reference to the
Instruction::executereuse contract beside the encoding-member/execute_implemitters inlib/python/amdisa/codegen/_generator.py(for example near line 4464). The new comment at line 1927 is beside the split-backend trampoline, away from where persistent executor state would be introduced.
It would help to pin the reuse contract with a few targeted regressions in emulation/rocjitsu/tests/instruction_cache_test.cpp:
- Exercise a memory instruction with no
DynamicInstStatethroughScopedReturn, then require a fresh decode. The existing exclusion tests use non-memory instructions, so they do not isolate theis_memory_op()check (decoded_instruction_cache.h:71). - Warm the CU cache before replacing its decoder, then require the replacement decoder to run at the same PC. Current replacement tests install the decoder before issue, so they do not exercise the invalidation in
compute_unit.h:1060. - Keep two waves resident in
VectorReadsCurrentExecAcrossDispatches(instruction_cache_test.cpp:479), assert distinct register bases, and step them with different values and EXEC masks. It currently halts each wave before launching the next, allowing first-fit allocation to reuse the same base. The DPP test covers distinct bases through direct execution, but not through the issue loop.
A trimmed, parameterized fresh-decode-versus-reexecution test would also protect the generator contract across ISAs. The supplemental probe found no mismatches across five ISA families and DPP/SDWA variants; making a representative subset permanent would catch future per-execution state accidentally stored in decoded objects. These are coverage gaps, not observed execution failures.
|
|
||
| /// @brief Return only completed, stateless instructions. | ||
| /// | ||
| /// @details Construct immediately after decode() with the same key and policy. |
There was a problem hiding this comment.
This would be safer if decode() returned an RAII lease capturing the key, reuse policy, and original fetch span, with access to the owned instruction for pipeline handoff.
The CU passes the same arguments correctly today (compute_unit.cpp:1512–1531), but ScopedReturn accepts them independently. In particular, a byte-identical copy of words satisfies the documented “same key” requirement while defeating borrows_fetch_buffer(): a VOPD instruction still points into the original decode buffer, which the copied span does not cover. A mismatched PC or policy can likewise return an object under the wrong key.
Capturing these inputs once would preserve the existing move/release and failed-execution paths without leaving this lifetime contract to every caller. This is an API safety suggestion; the current CU call site uses the correct span.
TheRock Submodule Bump ActivityNewest first
|
…ache (#14801) A quick win: VOPD instructions were never reused by the CU's decoded-instruction cache. Their constructor pointed raw_encoding_ at the caller's fetch words, and the cache does not keep instructions that borrow the fetch buffer, a deliberate exclusion from #12632. So every VOPD issue on RDNA3, RDNA3.5, RDNA4 and gfx1250 paid a full decode plus a heap allocation. This PR copies the encoding into the instruction (raw_words_ + memcpy), as other generated encodings already do, which lifts that exclusion. | Workload | Target | Cache hit before → after | VOPD share | Region before → after | Speedup | |---|---|---|---:|---:|---:| | flaggems amax-fp32-65536x4096 | gfx1250 | — | 45% | 11.38 → 8.20 s | **1.387x** | | torch batch-norm-train-128x256x4096 | gfx1250 | 73.0% → 85.0% | 12% | 29.96 → 28.51 s | 1.051x | | torch cumsum-fp32-8192x4096 | gfx1250 | 96.1% → 98.2% | 2.2% | 12.08 → 11.64 s | 1.038x | | rocprim merge-sort-u32-4m | gfx1201 | 95.1% → 95.8% | 0.65% | 22.67 → 21.86 s | 1.037x | | torch batch-norm-train (control, no VOPD) | gfx950 | 85.5% → 85.5% | 0% | 9.25 → 9.29 s | 0.996x | <details><summary>Full details for agents and humans</summary> #### Cause and fix | | Before | After | |---|---|---| | `Vopd::raw_encoding_` | points at the caller's fetch words | points at `raw_words_` inside the instruction | | Decoded-cache return | rejected (`borrows_fetch_buffer`) | kept, like any other ALU instruction | | Cost per VOPD issue | decode + heap allocation + free | cache hit | The change is in the generator template (`_generator.py`). The regenerated output touches only the eight `vopd.{h,cpp}` files for rdna3, rdna3_5, rdna4 and cdna5. The copy happens once `size_` is known: 8 bytes, or 12 with a literal or in the VOPD3 form. #### Tests *Changed.* `BorrowedVopdEncodingIsNotRetainedAcrossIssues` asserted the old borrowing behaviour, so it now covers two things separately: - `BorrowedEncodingIsNotRetainedAcrossIssues` keeps the cache's borrowed-encoding guard covered with a test-only decoder whose instruction borrows its words. This is the same pattern as `decoder_window_test.cpp`. - `VopdOwnsItsEncodingAndIsReusedAcrossIssues` checks all four VOPD targets, in the 8-byte form and the 12-byte literal form. The encoding is owned, the second issue reuses the decode (one decoder call), and the encoding is intact after the first fetch buffer is cleared. *Mutation check.* Restoring `raw_encoding_ = words` in the RDNA4 output alone fails the new test for RDNA4 only, in both forms. *Suite.* Full `ctest` passes. On the test host, `hsa_init_test` doesn't link against its older system HSA runtime, and that is unrelated to this change. `pre-commit` is clean. #### Performance *Method.* Region time from the workload's own markers. Each run is a fresh process. There is one warm-up run per build, then 5 paired rounds in alternating order. Each run has a thread budget of 1 and is pinned to 4 cores / 8 threads of an otherwise idle Ryzen 9950X3D2. Base is `9f686546da7`. Base run-to-run variation is 0.1–1.0% per workload. *Where it applies.* VOPD only exists on RDNA3, RDNA3.5, RDNA4 and gfx1250, so other targets are unaffected (see the gfx950 control). The gain scales with the workload's VOPD share. Reductions, argmax/amax, RNG, sort and norm kernels on gfx1250 use VOPD the most. #### Review focus 1. Is a copy (`raw_words_`) preferable to making the cache accept borrowers by rebinding `raw_encoding_` on return? 2. `raw_words_` duplicates `word0_`..`word2_`. Should `word*_` read from `raw_words_` instead, or is the small duplication fine in generated code? </details>
MT19937 initialization repeatedly executes a long scalar and LDS loop. The byte instruction cache avoids fetches, but each issue still decodes, constructs and destroys the same instruction and operand objects.
Retain completed non-memory instructions in a bounded, lazily allocated 256-entry cache per CU. Match PC, VMID and all fetched words, including literals. Preserve fetch permission, revocation and byte-cache checks before reuse, bypass reuse while debugging, and clear entries before replacing the decoder.
Allocate CU instructions on the heap without installing a decoder pool on the constructing thread. Keep ownership exclusive during issue and asynchronous execution, independently of worker and decoder lifetimes. Exclude memory operations, dynamic state, borrowed fetch-buffer encodings, failed execution and exception unwinding from retention.
Document decoded re-execution in Instruction and the execution generator. Cover current registers and EXEC, DPP delegate restoration, literal and VM changes, debugger writes, cache collisions and ownership transfer. Test unrelated allocator hooks and sequential worker lifetimes, and qualify the borrowed VOPD exclusion on RDNA3, RDNA3.5, RDNA4 and CDNA5.
On the reviewed base, all 162 selected cache, dispatch, async, matrix, wait-scoreboard and lane checks pass in native, forced-scalar and ASan/UBSan modes. The broad native suite retains only its two known DBT fixture failures. The registered launcher-handoff check and formatting checks pass.
Benchmarks use the reviewed cache implementation atop 0510825. Three alternating matched Release pairs against that lane-fixed base reduce median time for the exact gfx942 MT19937 jump kernel with 512 engines from 24.805 to 22.110 seconds at 32 dispatch threads (1.12x). Every raw state word matches the physical gfx1201 reference.
The complete exact-nightly 16,384-engine integer API case passes in 311.493 seconds with the original CI topology.
Matched IREE and controlled-compiler Gluon selections preserve every case outcome and compiled kernel hash. All 195 IREE cases pass; Gluon retains 129 passes, nine skips and its two baseline TDM gather failures.
Issue: https://github.com/ROCm/rocm-libraries/actions/runs/36740815382