Repository navigation
[rocjitsu] Rename gfx1250 to cdna5 for enums, configs, amdisa codegen helpers and generated symbols. (NFC) - #10137
Conversation
kuhar
left a comment
There was a problem hiding this comment.
It would be good to fix the naming-domain description in emulation/rocjitsu/docs/codegen.md:51 and line 92. The commands now use cdna5 as the logical generator key and the config loader uses cdna5 as the configuration arch, while these paragraphs still say both remain gfx1250. Please separate those from the MR ISA filename, GPU/runtime/ELF identity, and public DBT target identity that intentionally stay gfx1250.
The risk here is that arch_is_cdna() now excludes an enum named CDNA5, so a future one-line membership change would silently alter descriptor and wavefront behavior. At emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/isa_traits.h:86, please document why CDNA5 is intentionally outside this predicate and rename it to something more precise.
| /// @brief Compute Data Network Architecture 4 (CDNA4). | ||
| ROCJITSU_CODE_ARCH_CDNA4 = 3, | ||
| /// @brief Compute Data Network Architecture 5 (CDNA5). | ||
| ROCJITSU_CODE_ARCH_CDNA5 = 4, |
There was a problem hiding this comment.
We should preserve the existing numeric values in this exported C enum. At the current head, CDNA5 is inserted at 4, RDNA1 through RV64I shift, and CodeArchApiTest.PreservesExistingPublicEnumValues now asserts the shifted values. A client built against the previous header can therefore select a different architecture silently. Please assign CDNA5 the former GFX1250 value 11 and leave the established members unchanged.
| @property | ||
| def generated_arch_name(self) -> str | None: | ||
| return 'gfx1250' | ||
| return 'cdna5' |
There was a problem hiding this comment.
We should update the remaining generator gate for this logical-key rename. codegen/_generator.py:850 still checks for gfx1250, so CDNA5 regeneration selects OPR_SRC_VGPR_OR_INLINE where the checked-in K=128 WMMA constructors use OPR_SRC; the current profile-gate tests do not exercise this method. Please switch the gate to cdna5 and add a focused method-level test.
| @@ -3,7 +3,7 @@ | |||
| "num_threads": 1, | |||
| "exec_mode": "functional", | |||
| "vm": { | |||
| "arch": "gfx1250", | |||
| "arch": "cdna5", | |||
There was a problem hiding this comment.
We should rename this file to gfx1250_mi455x.json, following the convention used by the newer rdna3/4 configs.
| The manual commands below are useful for focused generator development and are | ||
| run from the rocjitsu project root. Set `MRISA` to the shared MR ISA directory | ||
| and `CDNA5_MRISA` to the out-of-tree CDNA5 MR ISA XML directory: | ||
|
|
||
| ```bash | ||
| MRISA=../../shared/machine-readable-isa/isa | ||
| CDNA5_MRISA=/path/to/cdna5-mrisa | ||
|
|
||
| python -m amdisa \ |
There was a problem hiding this comment.
Separately, we should advertise the new generator script here instead of invoking python directly. But that can go into another PR.
|
@kuhar, thanks for the review! I have also fixed the merge conflicts. |
kuhar
left a comment
There was a problem hiding this comment.
We should fix the remaining stale architecture string at emulation/rocjitsu/tests/execution_plugin_test.cpp:2857. PluginFixture still receives "gfx1250", which the renamed config parser rejects; the focused RaceDetectorPluginTest.DroppedAsyncLdsLaneDoesNotCreateLowAddressRace fails immediately with Missing or invalid arch in vm configuration. Please pass "cdna5" as the nearby D16 test now does.
| export ROCM_PATH | ||
| LAUNCHER="${ROCJITSU_BUILD_DIR}/tools/rocjitsu/rocjitsu" | ||
| CONFIG="${ROCJITSU_SOURCE_DIR}/configs/gfx1250.json" | ||
| CONFIG="${ROCJITSU_SOURCE_DIR}/configs/gfx1250_cdna5.json" |
There was a problem hiding this comment.
We should update this workflow to configs/gfx1250_mi455x.json and check that the config exists before launch. configs/gfx1250_cdna5.json is absent at this head, so both wrappers receive a missing config and the release corpus comparison cannot run.
|
|
||
| /// `MI450X` builtin agent (registry key `MI450X`), mirroring the | ||
| /// rocjitsu `gfx1250.json` config: `arch = gfx1250`, an | ||
| /// rocjitsu `gfx1250_cdna5.json` config: `arch = cdna5`, an |
There was a problem hiding this comment.
We should finish the MI450X producer rename here. agents.rs:135 still serializes vm.arch = "gfx1250" into the generated rj_config.json, but rocjitsu now accepts only cdna5, so MI450X sessions fail during config loading; the assertion at line 339 pins the stale value. Please use cdna5 in the builtin and test, and replace the missing gfx1250_cdna5.json references at lines 5 and 130 with gfx1250_mi455x.json.
| /// DBT call sites do not grow their own partial CDNA/RDNA switch statements. | ||
| [[nodiscard]] inline constexpr bool arch_is_cdna(rj_code_arch_t arch) { | ||
| /// | ||
| /// Currently CDNA5 is missing from this check. This is because CDNA5 |
There was a problem hiding this comment.
This would be safer if the comment stated the contract instead of saying CDNA5 is "missing." The brief still claims the whole CDNA family, while arch_is_cdna_4_or_lower() intentionally excludes CDNA5 because rocjitsu gives it RDNA4-derived descriptor properties. Please describe that exact CDNA1-4 boundary; adding CDNA5 here would silently change descriptor and wavefront decisions.
kuhar
left a comment
There was a problem hiding this comment.
LGTM but you may want to wait for Ada to sign off on the mirage changes
… helpers and generated symbols. (NFC) (#10137) ## Motivation Migrate architecture-level GFX1250 identifiers to the logical CDNA5 architecture identity. This separates the reusable architecture identity from concrete GFX1250 hardware and target-specific concepts. ## Technical Details * Replaces ROCJITSU_CODE_ARCH_GFX1250 with ROCJITSU_CODE_ARCH_CDNA5 across ISA dispatch, builders, code processing, configuration, VM, analysis, DBT, hooks, tools, and tests. * Makes cdna5 the canonical ISA provider and amdisa profile key. * Renames the simulator configuration to gfx1250_cdna5.json and updates its consumers. * Renames the Python ISA profile and CDNA5-specific code-generation helpers. * Renames generated matrix/WMMA helper symbols through cdna5_scaled_wmma_src1_size_bits. * Regenerates affected amdisa output. * Adapts tests split by the intervening CDNA5 simulator-test reorganization. This PR is limited to the logical CDNA5 architecture migration, including enum usage, provider/profile/config names, amdisa code-generation helpers, and generated matrix/WMMA symbols. ## Issue Tracking N/A ## Test Plan * Build the rocjitsu_tests target from build-nosan-debug. * Run focused CTest coverage for: * OperandSelectorDecodeTest * BinaryTranslatorE2E * Gfx1250ExecutionTest * Verify no tracked source references remain to the removed ROCJITSU_CODE_ARCH_GFX1250 enum. ## Test Result All good. ## Submission Checklist - [X] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. (cherry picked from commit c5fae8d)
…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>
… helpers and generated symbols. (NFC) (#10137) ## Motivation Migrate architecture-level GFX1250 identifiers to the logical CDNA5 architecture identity. This separates the reusable architecture identity from concrete GFX1250 hardware and target-specific concepts. ## Technical Details * Replaces ROCJITSU_CODE_ARCH_GFX1250 with ROCJITSU_CODE_ARCH_CDNA5 across ISA dispatch, builders, code processing, configuration, VM, analysis, DBT, hooks, tools, and tests. * Makes cdna5 the canonical ISA provider and amdisa profile key. * Renames the simulator configuration to gfx1250_cdna5.json and updates its consumers. * Renames the Python ISA profile and CDNA5-specific code-generation helpers. * Renames generated matrix/WMMA helper symbols through cdna5_scaled_wmma_src1_size_bits. * Regenerates affected amdisa output. * Adapts tests split by the intervening CDNA5 simulator-test reorganization. This PR is limited to the logical CDNA5 architecture migration, including enum usage, provider/profile/config names, amdisa code-generation helpers, and generated matrix/WMMA symbols. ## Issue Tracking N/A ## Test Plan * Build the rocjitsu_tests target from build-nosan-debug. * Run focused CTest coverage for: * OperandSelectorDecodeTest * BinaryTranslatorE2E * Gfx1250ExecutionTest * Verify no tracked source references remain to the removed ROCJITSU_CODE_ARCH_GFX1250 enum. ## Test Result All good. ## Submission Checklist - [X] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
Motivation
Migrate architecture-level GFX1250 identifiers to the logical CDNA5 architecture identity. This separates the reusable architecture identity from concrete GFX1250 hardware and target-specific concepts.
Technical Details
This PR is limited to the logical CDNA5 architecture migration, including enum usage, provider/profile/config names, amdisa code-generation helpers, and generated matrix/WMMA symbols.
Issue Tracking
N/A
Test Plan
Test Result
All good.
Submission Checklist