Repository navigation
[rocjitsu][amdisa][add-atomic 1/5] Generalize codegen for s_atomic_* instructions - #13102
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. |
accauble
changed the title
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
No issues from me, but an agent spotted a minor issue. So approving with a comment to address this:
[MINOR] GLC=0 s_atomic_dec triggers a false memory wait diagnostic. In the exact-head Release CI
run, all 12 cases of ReturnedRegisterRequiresLgkmWait failed the diagnostic-count assertion. The
functional value check passed: the non-returning atomic preserved s4, but reading s4 still produced
a wait warning. The precise point where the stale dependency entered the scoreboard was unresolved,
so confidence was medium in the defect and low in its internal cause.
The suggested fix was to suppress the result-register dependency when GLC=0 while retaining the
atomic’s LGKMCNT obligation. I found no other actionable issue. PR 12963 subsequently added GLC
dependent destination metadata and a regression test for this case; its reported rocjitsu CI checks
passed.
dc7b4b3 to
4800bb4
Compare
|
Rebased and added a commit to address minor unresolved findings from PR #12963 |
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Left an inline reply and a new comment
a44dd2f to
4fd3543
Compare
* Get op from _ATOMIC_OP_ENUM * Calculate width instead of assuming * Add python tests [rocjitsu][amdisa] Address #12963 review follow-ups PR #12963 had some minor tweaks requested: * Create a return-check helper for inst_.glc != 0 * Fold the scalar atomic mutation lambda into the L2 `atomic_rmw` callback * Use constants and builders `scalar_atomic_test.cpp` [rocjitsu][DBI] Set `elem_size` size and move test
4fd3543 to
4a48cd1
Compare
…instructions (#13102) ## Motivation NOTE: Based on PR #12963 which has not merged yet. This will remain in draft until that gets merged because I couldn't not submit a PR to that branch. PR #12963 added an `s_atomic_dec` implementation. The plan is to add the rest of the `s_atomic_*` instructions, so this PR generalizes that approach so all can be added. The intent of this PR is to generalize, not add additional behavior. That will be the next PRs in the stack. ## Issue Tracking Part of Issue #13025
TheRock Submodule Bump ActivityNewest first
|
Motivation
NOTE: Based on PR #12963 which has not merged yet. This will remain in draft until that gets merged because I couldn't not submit a PR to that branch.
PR #12963 added an
s_atomic_decimplementation. The plan is to add the rest of thes_atomic_*instructions, so this PR generalizes that approach so all can be added. The intent of this PR is to generalize, not add additional behavior. That will be the next PRs in the stack.Issue Tracking
Part of Issue #13025