Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Atomic.h #9707

Merged
merged 1 commit into from May 14, 2021
Merged

Remove Atomic.h #9707

merged 1 commit into from May 14, 2021

Conversation

JosJuice
Copy link
Member

The STL has everything we need nowadays.

I have tried to not alter any behavior or semantics with this change wherever possible. In particular, WriteLow and WriteHigh in CommandProcessor retain the ability to accidentally undo another thread's write to the upper half or lower half respectively. If that should be fixed, it should be done in a separate commit for clarity. One thing did change: The places where we were using += on a volatile variable (not an atomic operation) are now using fetch_add (actually an atomic operation).

Tested with single core and dual core on x86-64 and AArch64.

The STL has everything we need nowadays.

I have tried to not alter any behavior or semantics with this
change wherever possible. In particular, WriteLow and WriteHigh
in CommandProcessor retain the ability to accidentally undo
another thread's write to the upper half or lower half
respectively. If that should be fixed, it should be done in a
separate commit for clarity. One thing did change: The places
where we were using += on a volatile variable (not an atomic
operation) are now using fetch_add (actually an atomic operation).

Tested with single core and dual core on x86-64 and AArch64.
@lioncash
Copy link
Member

You absolutely love to see it =). I think with this, we finally have most of the volatile occurrences removed from the codebase (aside from one in DSP code off the top of my head)

@JosJuice
Copy link
Member Author

I've actually only removed 7 out of the 16 volatiles in SCPFifoStruct. The remaining ones shouldn't be hard to get rid of, but I'll handle them separately so the PR doesn't get too big.

{
Common::AtomicStore(_reg, (_reg & 0xFFFF0000) | lowbits);
reg.store((reg.load(std::memory_order_relaxed) & 0xFFFF0000) | lowbits,
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, is this still atomic in the end (with a seperate load then store)?
Then again, thats what the old code did, so same issue there...?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed not atomic. I mentioned this in the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I read that and dismissed it from my head while looking at LowPart/HighPart (only remembering the Low/High part).

@lioncash lioncash merged commit 964fed7 into dolphin-emu:master May 14, 2021
10 of 11 checks passed
@JosJuice JosJuice deleted the remove-atomic-header branch May 14, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants