Repository navigation
[rocjitsu] Define standalone decoder ownership - #10200
Merged
Merged
Conversation
✅ 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. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the rocJITsu C decoder API by ensuring decoder failures are contained within the C ABI boundary and by defining/implementing a clear ownership model for standalone decoded instructions (caller-owned with an explicit destroy API), including allocator/pool-safety for objects that can outlive their originating decoder.
Changes:
- Adds exception/oom handling and output-clearing semantics to
rj_code_decoder_create*andrj_code_decoder_decode, and introducesrj_code_inst_destroy()for standalone decode results. - Introduces
Instruction::ScopedHeapAllocationplus pool invalidation logic so C API allocations/deallocations remain safe even with ambient decoder pools and after decoder destruction. - Adds targeted tests to cover invalid encodings, invalid arguments, standalone ownership/lifetime, and interaction with an active decoder pool.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| emulation/rocjitsu/tests/decoder_c_api_test.cpp | New C API tests for exception containment, output clearing, ownership/lifetime, and pool interactions. |
| emulation/rocjitsu/tests/code/code_object_target_id_test.cpp | Extends target/registry tests to validate list-owned instruction lifetimes are independent of an unrelated active pool. |
| emulation/rocjitsu/tests/CMakeLists.txt | Adds the new decoder C API test to the test target. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/rj_decode.cpp | Adds exception boundary + OOM mapping, clears outputs on failures, forces heap allocation for standalone decodes, and implements rj_code_inst_destroy. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/instruction.h | Adds ScopedHeapAllocation and pool invalidation to prevent restoring/dereferencing destroyed pool pointers. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/decoder.h | Exposes Decoder::Pool and changes disable_pool() to a definition that can invalidate a specific pool. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/decoder.cpp | Updates destructor/disable behavior to invalidate allocator pool references (including saved scope state). |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/rj_code.cpp | Forces heap allocation for decoded instruction list/basic-block list creation and ensures destruction runs with heap allocation active. |
| emulation/rocjitsu/lib/rocjitsu/include/rocjitsu/code/rj_code.h | Documents standalone instruction ownership and adds the public rj_code_inst_destroy API; updates decoder create/decode contracts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
atgutier
reviewed
Aug 15, 2026
atgutier
left a comment
atgutier
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Just a suggestion on organization.
atgutier
approved these changes
Aug 16, 2026
atgutier
pushed a commit
that referenced
this pull request
Aug 19, 2026
Contain decoder failures inside the C API and clear writable outputs on every failure path. Document successful standalone decode results as caller-owned, provide a matching destroy API, and cover invalid encodings plus leak-free lifetime behavior. Fixes #8810 Fixes #8814 (cherry picked from commit 5c8e33e)
1 task done
atgutier
added a commit
that referenced
this pull request
Aug 20, 2026
…t calls (#10444) ## Motivation hipTensor and rocThrust gfx1250 objects are refused at code-object load time with "indirect branch or call target recovery is not implemented for relocated kernel text" on `s_swap_pc_i64`. A refused object leaves HIP holding an invalid kernel handle, so the failure surfaces as a segfault rather than a diagnostic. The fix is upstream as #10376; this backports it to `release/bkc/therock-10.1-20260811`. ## Commits Cherry-Picked | # | Upstream | PR | Backport | Subject | |---|----------|-----|----------|---------| | 1 | `b6d838028e` | #10197 | `846fb16d12` | [rocjitsu][docs] Refresh architecture support documentation | | 2 | `5c8e33e391` | #10200 | `db5c58b1be` | [rocjitsu] Define standalone decoder ownership | | 3 | `be1f91998f` | #10199 | `ed102baf1c` | [rocjitsu] Apply async IOFFSET to LDS addresses | | 4 | `fda635e5b4` | — | `ad32ff7c18` | [rocjitsu] Extract per-scope rollback and object-level checks from translate() | | 5 | `8f75eb781e` | — | `4c0cb05207` | [rocjitsu] Emit leading V_NOPs ahead of a gfx1250 MODE write | | 6 | `00aa1e260f` | #9941 | `ded43b1787` | fix(rocjitsu): disable zero-count TDM descriptors | | 7 | `c810647609` | #10192 | `50ca7f1732` | [rocjitsu] Apply MUBUF LDS-load instruction offset to the LDS address | | 8 | `1d7866269d` | #9560 | `2e6bf26943` | [rocjitsu] code rewrite condition validator | | 9 | `95b6c410bb` | #10175 | `8418901076` | [rocjitsu] Fix ds_read_b64_tr_b16 lane layout in gfx950 simulator | | 10 | `bfaebcfb5c` | #10240 | `c4eac1e1f7` | [rocjitsu][util] Add result and diagnostic utilities | | 11 | `dbb7221fa7` | #9775 | `cdd0993a5f` | fix(rocjitsu): route tensor DMA through wave VMID | | 12 | `2779247222` | #9779 | `7bfeba6f5d` | feat(rocjitsu): Lazy vgpr allocation system | | 13 | `a659172a31` | #9936 | `180cf00325` | fix(rocjitsu): honor TDM tensor bounds across iterations | | 14 | `60c29efb39` | #10204 | `370bfb9796` | [rocjitsu] Fix gfx950 decoder extension handling | | 15 | `c1dba564e7` | #10345 | `536110a083` | [rocjitsu] Model gfx1250 wave ABI state | | 16 | `7f71be9f32` | #10346 | `3f922ad5f2` | [rocjitsu] Model gfx1250 barrier synchronization | | 17 | `bea7ac8937` | #10368 | `7b5ecf6c7b` | [rocjitsu] Replace decoder exceptions with results | | 18 | `f2e878694e` | #10371 | `8cf750d23f` | [rocjitsu] Dispatch operand validation by type | | 19 | `dd619bed77` | #9735 | `1a612c47c0` | fix(rocjitsu): respect same-wave LDS ordering | | 20 | `c5fae8d2b2` | #10137 | `4360c4b978` | [rocjitsu] Rename gfx1250 to cdna5 for enums, configs, amdisa codegen helpers and generated symbols. (NFC) | | 21 | `b05a587f89` | #10274 | `bfb5f9fab0` | [rocjitsu] Align CDNA4 MI350X configs with captured topology | | 22 | `f81f9d40db` | #10362 | `8177b5725c` | feat(rocjitsu): cdna5 OOB behavior implementation | | 23 | `53a939bd00` | #10283 | `141b109432` | [rocjitsu] add per-XCD workgroup dispatch observability | | 24 | `a0fb11bc17` | #10414 | `6bb4c5a959` | [rocjitsu] Refactor test encoding handling in execution tests (NFC) | | 25 | `e5109e315e` | #9844 | `322ac26b44` | feat(emulation): debug emulated GPU kernels with real ROCgdb | | 26 | `efa24f23c9` | — | `c3c9641073` | rocjitsu: Point the XCD distribution test at the config that exists | | 27 | `8b7763ef5e` | #10376 | `c3f419555a` | [rocjitsu] Translate gfx1250 lane-banked and kernarg-supplied indirect calls | ## Issue Tracking [ROCM-29436] ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. [ROCM-29346]: https://amd-hub.atlassian.net/browse/ROCM-29346?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [ROCM-29436]: https://amd-hub.atlassian.net/browse/ROCM-29436?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Signed-off-by: Tony Gutierrez <anthony.gutierrez@amd.com> Signed-off-by: hanhanW <hanhan0912@gmail.com> Co-authored-by: Jakub Kuderski <jakub@nod-labs.com> Co-authored-by: James Newling <james.newling@gmail.com> Co-authored-by: Fei Kou <feikou@meta.com> Co-authored-by: Austin Lu <austin.lu@amd.com> Co-authored-by: Fei Kou <feikou@anthropic.com> Co-authored-by: Erick Ochoa Lopez <eochoalo@amd.com> Co-authored-by: Han-Chung Wang <hanhan0912@gmail.com> Co-authored-by: Rosa, Ada <arosa@amd.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Contain decoder failures inside the C API and clear writable outputs on every failure path.
Document successful standalone decode results as caller-owned, provide a matching destroy API, and cover invalid encodings plus leak-free lifetime behavior.
Fixes #8810
Fixes #8814