Repository navigation
[rocjitsu][amdisa] Prevent scalar buffer mem op if range not in bounds - #14922
Conversation
* For a scalar buffer memory op, we checked to see if the first byte was in range, but not the rest of the load. Add that check
✅ 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. |
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would help to document the two alignment masks at emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/addr_calc_scalar.h:44-49: align_mask rounds the returned address, while size_align_mask rounds the byte bound used for per-DWORD masking. GFX9 now passes 3 for both; RDNA4 passes its access alignment, while RDNA1 through RDNA3.5 retain the default zero size mask. A short note that zero leaves the bound unrounded, and that those RDNA callers are unchanged by this fix, would make the policy distinction clear without implying their hardware behavior was validated here.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It might help to require each scalar_buffer_address caller to pass its bound alignment explicitly (emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/isa/arch/amdgpu/shared/addr_calc_scalar.h:56). Omitting size_align_mask silently selects the first-byte rule, which would give the wrong result for a new caller that needs whole DWORDs. The four RDNA1–3.5 callers could pass /*align_mask=*/3, /*size_align_mask=*/0. A reference at the RDNA3 call site to ISA §§8.1.2 and 8.4.1 would explain the unrounded bound without implying it was tested on hardware.
rocjitsu post-commit fuzz passedThe 32-worker multiplexer campaigns for commit
|
## Motivation This models the `s_buffer_atomic_*` instructions (Issue #13025). The implementation is very similar to the `s_atomic` implementation except for how smem_calculate_address computes the address and an extra bounds check. Requires a quick fix in PR #14922. I also found another possible bug with how the buffer memory accesses are checked for the debugger. That is scoped as a follow-up since it existed before this change. (PR #14995)
TheRock Submodule Bump ActivityNewest first
|
Motivation
For a scalar buffer memory op simulation, we checked to see if the first byte was in range, but not the rest of the load. This adds that check and tests. Related to Issue #13025.