Repository navigation
fix(rocjitsu): honor TDM tensor bounds across iterations - #9936
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. |
a00b26f to
2dc7cbf
Compare
a69c800 to
8518f51
Compare
8518f51 to
cd6e42f
Compare
kuhar
left a comment
There was a problem hiding this comment.
We should apply one zero-extent rule consistently to both copy paths. At emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/tensor_dma.h:283, copy_gather_tensor still treats tensor_dims[0] == 0 as unbounded, while the dense path now treats it as empty. A rank-1 gather descriptor with a nonzero tile dimension and valid_indices == 1 parses and passes validation with a zero extent, so it can still copy past the declared tensor extent. Please either apply the empty-extent rule to gather and add a regression, or document why gather uses a different descriptor meaning.
cd6e42f to
4319816
Compare
|
I believe all comments have been addressed. |
There was a problem hiding this comment.
[MAJOR]
tensor_dma.h:222 — Null D2 is incorrectly rejected instead of being interpreted as a zero-filled
descriptor group
Reasoning: s_null for an optional descriptor group has the same effective value as four zero SGPRs.
For an active rank-three tile, this produces tensor_dims[2] == 0. Under this PR’s corrected empty-
extent behavior, a load should zero-fill the tile, a store should suppress global writes, and an
enabled completion barrier should still arrive. The new check instead throws UnimplementedInst,
aborting those defined behaviors.
Evidence:
-
read_sgpr_group(..., allow_null=true) already converts s_null into a zero-filled group at lines
42–46. -
parse_descriptor() consequently obtains tensor_dims[2] == 0 at line 137.
-
Rank remains three because the active tile_dims[2] is independently encoded in D1 and used by
rank(). -
The public LLVM backend lowers all-zero D2/D3 groups to the two-group form whose VADDR2/VADDR3
encodings are s_null; the selector makes this decision solely from whether those groups are zero. -
The new bounds predicate at lines 331–336 would correctly mask the zero third extent if execution
reached it. -
The current regression /tmp/pr9936-current/emulation/rocjitsu/tests/gfx1250_sim_test.cpp:3883
codifies the incorrect exception. -
Because the exception occurs before arrive_atomic_barrier(), an enabled completion barrier would
also be lost.
Suggested fix: Remove d2_present and the presence-based rejection. Replace
TensorDmaRankThreeRejectsNullD2Operand with decoded-instruction regressions verifying:
- A load with an active third tile dimension and null D2/D3 zero-fills LDS.
- The corresponding store leaves global memory unchanged.
- An enabled completion barrier still arrives.
- TENSORCNT is released normally.
Confidence: high.
4319816 to
459c14f
Compare
|
Addressed atgutier’s major review in |
|
Just to be transparent: my last comment here was pushed by an agent without my knowing. Apologies for the noise, I try not to let them write to network without my explicit permission. I'm still iterating on this PR... |
459c14f to
19024e9
Compare
19024e9 to
afc57d6
Compare
|
This doesn't build for me with gcc 13: |
afc57d6 to
b08f0b7
Compare
b08f0b7 to
cb2a417
Compare
|
Marking all review comments as done, I'll follow up on the docs related comments later. |
## Summary This change fixes tensor-DMA bounds contracts exposed while reducing the stock gfx1250 TensileLite TDM tail failure tracked by #9935. It is independent of the VMID correction in #9775 at the source level. Both changes are needed to advance the original workload through the currently known rocJITsu model gaps. ## Related producer-side evidence - ROCm/rocm-libraries#7976 introduced TDM iterate mode and the `tdm_gfx1250.yaml` configuration lineage that exposed this gap. - ROCm/rocm-libraries#8406 contains the same affected configuration after iterate mode was enabled and reports passing FFM and B0 validation. This is file-level evidence rather than a published per-solution trace. - ROCm/rocm-libraries#9410 added iterate-mode edge cases with waves that have zero valid rows specifically to exercise descriptor bounds clamping, and reports passing FFM validation. ## Failure before this fix Iterate mode advanced the starting global address for each repeated tile, but rocJITsu continued to apply bounds using tile-local coordinates. A final row outside the tensor was therefore copied into LDS instead of being zero-filled. An active tensor extent of zero was treated as unbounded. The complete dense or gather tile could therefore be copied instead of being masked. ## Fixes ### Include iterate-mode address advancement in tensor bounds The model decomposes each advancing iteration's linear global-address increment into an absolute tensor-coordinate origin, then applies one bounds predicate to that origin plus the tile-local coordinate. This handles sub-row advances, row advances, and rank-3 plane advances through the same path. Overlapping-stride layouts are rejected only when multiple iterations actually advance the global origin; repeated-origin descriptors do not require inverse coordinate decomposition. ### Treat zero active tensor extents as empty For an active dense or gather tile dimension, tensor extent zero masks the complete dimension. Loads zero-fill the affected LDS elements and stores suppress the corresponding global writes. Optional null descriptor groups retain their architectural zero-filled value. A rank-3 tile with null D2 therefore has a zero third tensor extent: it masks the transfer without aborting execution, still completes an enabled barrier, and releases TENSORCNT normally. ### Avoid unnecessary origin decomposition Non-iterating descriptors use an all-zero origin directly. This avoids inverse stride decomposition, including accepted rank-5 layouts whose large default strides can wrap. ## Test coverage Focused decoded-instruction regressions verify: - partial final-row load zero-fill and corresponding store suppression; - active zero-length dense and gather extents; - null-D2 rank-3 load masking and store suppression; - completion-barrier arrival and TENSORCNT release for null D2; - sub-row, row, and rank-3 plane iteration origins; - rejection of advancing overlapping-stride layouts; - acceptance of repeated-origin overlapping-stride layouts; and - safe non-iterating rank-5 execution. The complete `Gfx1250ExecutionTest.TensorDma*` test set and changed-file pre-commit checks pass locally. ## Workload status This PR intentionally does not claim to fix the complete TensileLite reproducer. It removes independently testable descriptor-model discrepancies and gets the workload closer to the unresolved tail-loop semantic question. ## Issue tracking Progress toward #9935. (cherry picked from commit a659172)
…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>
Summary
This change fixes tensor-DMA bounds contracts exposed while reducing the stock gfx1250 TensileLite TDM tail failure tracked by #9935.
It is independent of the VMID correction in #9775 at the source level. Both changes are needed to advance the original workload through the currently known rocJITsu model gaps.
Related producer-side evidence
tdm_gfx1250.yamlconfiguration lineage that exposed this gap.Failure before this fix
Iterate mode advanced the starting global address for each repeated tile, but rocJITsu continued to apply bounds using tile-local coordinates. A final row outside the tensor was therefore copied into LDS instead of being zero-filled.
An active tensor extent of zero was treated as unbounded. The complete dense or gather tile could therefore be copied instead of being masked.
Fixes
Include iterate-mode address advancement in tensor bounds
The model decomposes each advancing iteration's linear global-address increment into an absolute tensor-coordinate origin, then applies one bounds predicate to that origin plus the tile-local coordinate.
This handles sub-row advances, row advances, and rank-3 plane advances through the same path. Overlapping-stride layouts are rejected only when multiple iterations actually advance the global origin; repeated-origin descriptors do not require inverse coordinate decomposition.
Treat zero active tensor extents as empty
For an active dense or gather tile dimension, tensor extent zero masks the complete dimension. Loads zero-fill the affected LDS elements and stores suppress the corresponding global writes.
Optional null descriptor groups retain their architectural zero-filled value. A rank-3 tile with null D2 therefore has a zero third tensor extent: it masks the transfer without aborting execution, still completes an enabled barrier, and releases TENSORCNT normally.
Avoid unnecessary origin decomposition
Non-iterating descriptors use an all-zero origin directly. This avoids inverse stride decomposition, including accepted rank-5 layouts whose large default strides can wrap.
Test coverage
Focused decoded-instruction regressions verify:
The complete
Gfx1250ExecutionTest.TensorDma*test set and changed-file pre-commit checks pass locally.Workload status
This PR intentionally does not claim to fix the complete TensileLite reproducer. It removes independently testable descriptor-model discrepancies and gets the workload closer to the unresolved tail-loop semantic question.
Issue tracking
Progress toward #9935.