Repository navigation
Conversation
copy_dense_tensor() bounds-checked each tile coordinate against the descriptor's tensor extent for that dimension, but skipped the check when the extent was zero. Zero cannot mean unbounded here: parse_descriptor() reads a real field for every dimension below rank(), and rank() for a dense descriptor comes from the tile dimensions, so there is no dimension inside the rank whose extent went unset. Zero means the tensor is empty in that dimension and every coordinate in it is out of bounds. The consequence was that a descriptor with nothing left to copy -- the last prefetch of a pipelined loop, where the base has advanced past the end of the tensor and the remaining extent has counted down to zero -- copied a full tile from beyond the allocation. A load pulled whatever followed the tensor in memory into LDS; a store wrote the tile over memory the guest never named. Bound every dimension below the rank unconditionally. The out-of-bounds elements then take the path copy_bytes() already had: zero into LDS on a load, nothing written on a store, which is what the hardware does with a tile that hangs off the end. copy_gather_tensor() keeps its check as it is. A gather descriptor's rank is derived from the tensor extents themselves, so a zero there genuinely does mean the dimension is absent. Tested by Gfx1250ExecutionTest.TensorDmaEmptyTensorExtentReadsNothing, which issues a rank-2 descriptor whose dim1 extent is zero and asserts the load leaves LDS zeroed and the store leaves the destination at its sentinel. Before the fix the test saw the out-of-bounds rows in LDS and the tile written over the sentinel.
✅ 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
Author
|
Superseded. |
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.
Motivation
At origin/develop, copy_dense_tensor() in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/tensor_dma.h wrote
if (desc.tensor_dims[dim] != 0 && coord >= desc.tensor_dims[dim]) in_bounds = false;, so a zero per-dimension tensor extent disabled the bounds check for that dimension and the tile was treated as unbounded there. For a dense descriptor that is wrong: rank() is derived from tile_dims, and parse_descriptor() populates a real extent field for every dimension below that rank, so zero cannot be an "unset" sentinel — it means the tensor is empty in that dimension and every coordinate is out of bounds. The fix drops the zero guard, evaluatingcoord >= desc.tensor_dims[dim]unconditionally; out-of-bounds elements then take the path copy_bytes() already had (zero into LDS on a load, nothing written on a store). copy_gather_tensor() is correctly left alone, since a gather descriptor's rank is derived from the extents themselves. The source hunk is substantively identical to the reference fix 24fce1a489, and the new test reproduces the defect exactly.What a guest program saw before this change: A guest program whose descriptor had counted an extent down to zero — the last prefetch of a pipelined loop, where the base has advanced past the end of the tensor and nothing is left behind it — got a full tile copied from beyond the allocation. A TENSOR_LOAD_TO_LDS pulled whatever followed the tensor in device memory into LDS instead of zeros; a TENSOR_STORE_FROM_LDS wrote the tile over memory the guest never named. On the motivating case (mxfp_fa_cdna5, a 256x8 tile) the out-of-bounds read reportedly took the emulator down with a host SIGSEGV.
Technical Details
Changed files:
/tmp/rj-fix-tensor-extent/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/tensor_dma.h/tmp/rj-fix-tensor-extent/emulation/rocjitsu/tests/cdna5_tensor_dma_test.cppIssue Tracking
GitHub issue: #10327
Test Plan
Written test-first: the regression test was added and observed to fail against
developbefore any source change, then the fix was applied and the testre-run. New tests:
Gfx1250ExecutionTest.TensorDmaEmptyTensorExtentReadsNothingFailure observed before the fix:
Test Result
After the fix the new tests pass and the full suite is green:
Notes: VERDICT: the fix is correct, minimal, and genuinely validated. I made no changes and did not amend the commit. Criticisms below are all outstanding but minor; none justify churning the change.
WHAT I VERIFIED MYSELF
git diff origin/developis exactly two hunks, 62 insertions / 1 deletion. Confined to emulation/; projects/ untouched, no submodule bump. Thedesc.tensor_dims[dim] != 0 &&guard is indeed present at origin/develop.