Skip to content

[rocjitsu][amdisa][add-atomic 2/5] Add the s_atomic_dec_x2 instruction - #13104

Merged
kuhar merged 1 commit into
developfrom
users/accauble/rj-s-atomic-dec-x2
Oct 21, 2026
Merged

[rocjitsu][amdisa][add-atomic 2/5] Add the s_atomic_dec_x2 instruction#13104
kuhar merged 1 commit into
developfrom
users/accauble/rj-s-atomic-dec-x2

Conversation

@accauble

@accauble accauble commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

A previous PR added s_atomic_dec but not s_atomic_dec_x2. This PR adds the latter and makes sure that the generalization made earlier in this stack is enough. This adds functionality so I am giving its own PR. The rest of the s_atomic_* instructions will be added in the next PR.

Issue Tracking

Part of Issue #13025

@accauble
accauble added this pull request to stack #13105 October 8, 2026 13:42
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@accauble accauble changed the title [rocjitsu][amdisa][add-atomic 1/5] Add the s_atomic_dec_x2 instruction [rocjitsu][amdisa][add-atomic 2/5] Add the s_atomic_dec_x2 instruction Oct 8, 2026
@accauble
accauble force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from 30b9795 to 5b4fb9f Compare October 8, 2026 13:50
This was referenced Oct 8, 2026
Merged
Open
kuhar
kuhar reviewed Oct 15, 2026
View reviewed changes

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to make the new test tables easier to read and extend in emulation/rocjitsu/tests/scalar_atomic_test.cpp (minor suggestions):

  • At line 203, use std::to_array<ReadModifyWriteCase> so the table length follows its entries. That removes the separately maintained 15 and avoids silently adding a default-initialized case when entries are removed.
  • At lines 520 and 543, give the observer record named fields such as mnemonic, elements_per_lane, and returns_value. The expected rows would then explain what 2 and false mean without referring back to emplace_back.

Could we extend FailedAtomicDoesNotWriteBackOrLeakCounter and FailedCompareExchangeDoesNotWriteBackOrLeakCounter over kDecrementForms, using the width-aware memory/register helpers? They still exercise only the 32-bit instruction at emulation/rocjitsu/tests/scalar_atomic_test.cpp:395 and :451.

The new X2 path returns two dwords and holds two LGKM tokens. Keeping both widths in these fault tests would protect the full register pair and counter cleanup on load/CAS failure. The new misalignment test exits before either operation, so it does not cover these cases.

Comment thread emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/memory_pipeline.cpp Resolved
Comment thread emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py Resolved
Comment thread emulation/rocjitsu/tests/scalar_atomic_test.cpp Resolved
@accauble
accauble force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from 5b4fb9f to 25fe0f9 Compare October 20, 2026 15:28
@accauble

accauble commented Oct 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I believe I have made the requested changes

@accauble
accauble requested a review from kuhar October 20, 2026 15:30
@accauble
accauble marked this pull request as ready for review October 20, 2026 15:30
@accauble
accauble requested a review from a team as a code owner October 20, 2026 15:30
kuhar
kuhar reviewed Oct 20, 2026
View reviewed changes

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extend DecodedDependenciesFollowReturnBit at emulation/rocjitsu/tests/scalar_atomic_test.cpp:498 over kDecrementForms? It still decodes only s_atomic_dec and expects one data SGPR and one LGKM token. For X2, please assert the two-SGPR source and GLC-gated destination ranges, plus counter_increment() == 2 for both GLC values.

The runtime tests cover the counter and high-dword return, but do not directly check the X2 InstDefUse source range. Exercising both widths here would protect the decode metadata that this PR generalizes.

Comment thread emulation/rocjitsu/tests/scalar_atomic_test.cpp Resolved
Comment thread emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/memory_pipeline.cpp Resolved
@accauble
accauble force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from 814129f to e0aeaf9 Compare October 20, 2026 17:06
kuhar
kuhar approved these changes Oct 20, 2026
View reviewed changes

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ROCm ROCm deleted a comment from therock-pr-bot Bot Oct 20, 2026
@accauble
accauble force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from e0aeaf9 to ed6d4d1 Compare October 20, 2026 20:04
@accauble
accauble force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from ed6d4d1 to 185ec0c Compare October 20, 2026 20:39
Base automatically changed from users/accauble/rj-generalize-s-atomic to develop October 21, 2026 01:14
@accauble @kuhar
[rocjitsu][amdisa] Add s_atomic_dec_x2
c935dec 
* X2 atomics take 2 LGKM tokens
* Report scalar atomic to plugins and debugger as one element of
  the access width, matching vector atomics
@kuhar
kuhar force-pushed the users/accauble/rj-s-atomic-dec-x2 branch from 185ec0c to c935dec Compare October 21, 2026 01:14
@kuhar
kuhar merged commit 053c912 into develop Oct 21, 2026
74 checks passed
@kuhar
kuhar deleted the users/accauble/rj-s-atomic-dec-x2 branch October 21, 2026 10:49
kuhar pushed a commit that referenced this pull request Oct 21, 2026
@accauble
[rocjitsu][amdisa][add-atomic 3/5] Add rest of s_atomic_* instruction…
0bd483d 
…s (except cmpswap) (#13110)

## Motivation

Adds the rest of the `s_atomic_*` instructions (except
`s_atomic_cmpswap*` which will be treated differently) in the same way
that `s_atomic_dec` and `s_atomic_dec_x2` were added in PR #12963 and PR
#13104.

## Issue Tracking

Part of Issue #13025.
dgaliffiAMD pushed a commit that referenced this pull request Oct 21, 2026
@accauble @dgaliffiAMD
[rocjitsu][amdisa][add-atomic 2/5] Add the s_atomic_dec_x2 instruction (
59435d8 
#13104)

## Motivation

A previous PR added `s_atomic_dec` but not `s_atomic_dec_x2`. This PR
adds the latter and makes sure that the generalization made earlier in
this stack is enough. This adds functionality so I am giving its own PR.
The rest of the `s_atomic_*` instructions will be added in the next PR.

## Issue Tracking

Part of Issue #13025
dgaliffiAMD pushed a commit that referenced this pull request Oct 21, 2026
@accauble @dgaliffiAMD
[rocjitsu][amdisa][add-atomic 3/5] Add rest of s_atomic_* instruction…
1419ae8 
…s (except cmpswap) (#13110)

## Motivation

Adds the rest of the `s_atomic_*` instructions (except
`s_atomic_cmpswap*` which will be treated differently) in the same way
that `s_atomic_dec` and `s_atomic_dec_x2` were added in PR #12963 and PR
#13104.

## Issue Tracking

Part of Issue #13025.
@systems-assistant

systems-assistant Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atgutier atgutier atgutier approved these changes
@kuhar kuhar kuhar approved these changes

Assignees

No one assigned

Labels

organization: ROCm project: rocjitsu

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@accauble @kuhar @atgutier