Repository navigation
fix(rocjitsu): Sign-extend the gfx1250 GLOBAL saddr VGPR offset - #10847
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
Fixes incorrect address calculation for gfx1250 GLOBAL saddr operations by properly sign-extending the 32-bit VGPR offset (and applying any scaling using signed arithmetic), preventing negative strides from being interpreted as large positive offsets and silently dropping stores/loads in daemon mode.
Changes:
- Update gfx1250 GLOBAL saddr address calculation to sign-extend the VGPR offset and multiply it with signed semantics.
- Add a regression test that writes through a negative VGPR offset and verifies the marker lands below the base while the base remains unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/cdna5/addr_calc.cpp | Sign-extend GLOBAL saddr VGPR offsets (and scale them in signed arithmetic) when forming per-lane addresses. |
| emulation/rocjitsu/tests/cdna5_dispatch_memory_test.cpp | Add a regression test validating negative GLOBAL saddr VGPR offsets write below the base on gfx1250. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
atgutier
force-pushed
the
users/agutierr/gfx1250-signed-gvs-offset
branch
from
August 27, 2026 20:10
391037b to
ac6cdef
Compare
atgutier
force-pushed
the
users/agutierr/gfx1250-signed-gvs-offset
branch
from
August 27, 2026 20:33
ac6cdef to
0592323
Compare
kuhar
approved these changes
Aug 28, 2026
12 tasks done
The VGPR half of a GLOBAL saddr address is a signed 32-bit offset on gfx1250, where every earlier target zero-extends it. LLVM gates the difference on hasSignedGVSOffset, matching a sign extension rather than a zero extension and selecting the signed multiply-add for the scaled form, so it will place a negative value there whenever an access walks backwards from its base. The address calculation zero-extended it, which does not move an offset of -4 one dword below the base but 4 GiB above it. Nothing is mapped that far out, so in daemon mode the access fell through to the client-memory bridge, failed there, and the store was discarded -- the symptom is quietly missing data rather than a fault. Widen the offset through int32_t, and apply the scale to the widened value so the multiply is signed too. A two-element torch.flip reproduces the original: the lane carrying the -4 stride lost its store and the destination kept the zero it was allocated with, while the lane at offset zero wrote correctly. A single element passes either way because its only offset is zero, which is why this survived until a reversed iteration order reached the simulator. The regression test stores a marker through a negative offset and checks that it lands below the base and that the base itself is untouched. Closes #10757 Signed-off-by: Tony Gutierrez <anthony.gutierrez@amd.com>
atgutier
force-pushed
the
users/agutierr/gfx1250-signed-gvs-offset
branch
from
August 29, 2026 19:55
0592323 to
dbc6f33
Compare
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
The VGPR half of a GLOBAL saddr address is a signed 32-bit offset on gfx1250, where every earlier target zero-extends it. LLVM gates the difference on hasSignedGVSOffset, matching a sign extension rather than a zero extension and selecting the signed multiply-add for the scaled form, so it will place a negative value there whenever an access walks backwards from its base. The address calculation zero-extended it, which does not move an offset of -4 one dword below the base but 4 GiB above it. Nothing is mapped that far out, so in daemon mode the access fell through to the client-memory bridge, failed there, and the store was discarded -- the symptom is quietly missing data rather than a fault.
Widen the offset through int32_t, and apply the scale to the widened value so the multiply is signed too. A two-element torch.flip reproduces the original: the lane carrying the -4 stride lost its store and the destination kept the zero it was allocated with, while the lane at offset zero wrote correctly. A single element passes either way because its only offset is zero, which is why this survived until a reversed iteration order reached the simulator. The regression test stores a marker through a negative offset and checks that it lands below the base and that the base itself is untouched.
Closes #10757