Repository navigation
[rocjitsu][amdisa] Model s_buffer_atomic_* instructions - #14972
Conversation
github-actions
Bot
added
documentation
6778aa9 to
efdadd8
Compare
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
It would help to describe load_dword_mask in terms of set and clear bits. At addr_calc_scalar.h:31–32 and mem_state.h:98–99, a set bit selects a DWORD for access. A clear bit makes a load return zero or skips a store. The scalar_buffer_address comment at addr_calc_scalar.h:44–52 could then explain the atomic case. An aligned element needs every DWORD in range, while a misaligned one keeps the full mask so it can fault. Parenthesizing the || condition at line 69 would make those two paths easier to spot.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Added some comments and an inline reply
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Original anchor: emulation/rocjitsu/tests/scalar_atomic_test.cpp:725
Waitcnt now pins that buffer atomics consume the three V# words (ScalarBufferAtomicChecksItsConsumedDescriptorWords). InstDefUse still only sees the 64-bit s_atomic address here.
Buffer constructors emit a 128-bit sbase, the same shape smem_sbase_operand_test.cpp already pins for s_buffer_load_dwordx4 -> s[4:7]. That test exists because a scaled-SBASE miss once reached the probe-call planner. Looping buffer_atomic_opcode here with width 4, or adding one s_buffer_atomic_inc case there, would put liveness and spill on the same V# path as loads instead of leaving them with only the 64-bit address.
This is a small test addition, not a model change. Worth doing in this PR while the constructors are new, so a later consumer of InstDefUse does not have to rediscover that buffer atomics are a V#, not s[0:1].
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
One comment, plus an inline reply
✅ 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. |
* Very similar to `s_atomic` implementation except for how smem_calculate_address computes the address and an extra bounds check
* `s_buffer_atomics` get `modifiers.buffer_resource = &sbase` and `scalar_buffer_words = 3` like buffer loads. * Prevent future additions to `s_buffer_atomic` from being parsed as something else * Test change * Comment clarifications
4039d70 to
8994908
Compare
rocjitsu post-commit fuzz passedThe 32-worker multiplexer campaigns for commit
|
TheRock Submodule Bump ActivityNewest first
|
Motivation
This models the
s_buffer_atomic_*instructions (Issue #13025). The implementation is very similar to thes_atomicimplementation 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)