Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Mar 3, 2020

From discussion in #33042 make Interlocked.Or and Interlocked.And return the original value instead of the updated one.

There aren't yet any usages in libraries that look at the returned value.

cc: @jkotas, @GrabYourPitchforks, @tannergooding

@MihaZupan MihaZupan added this to the 5.0 milestone Mar 3, 2020
@MihaZupan MihaZupan requested a review from stephentoub March 3, 2020 13:23
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM.

Seems a bit unfortunate that whether the new or original value is returned differs based on the operation, but this does match what the Windows interlocked APIs are doing. It looks to also match what ARM64 encodes for their atomic instructions, but not quite what x86 does (outside the special "XADD - Exchange and Add" instruction).

@stephentoub stephentoub merged commit 9b74d82 into dotnet:master Mar 3, 2020
@EgorBo
Copy link
Member

EgorBo commented Mar 5, 2020

I find it a bit confusing honestly.
Because Interlocked.Add returns new value, and Interlocked.And and Interlocked.Or both return old value.
I mean I'd prefer them all to return "original value" but we can't change behavior for Add.

@stephentoub
Copy link
Member

I find it a bit confusing honestly.

That's why I did it as the new value initially, for consistency with Add.

But with Add, you can always compute the original value from the new value; that's not the case for And and Or.

monojenkins pushed a commit to monojenkins/mono that referenced this pull request Mar 5, 2020
These APIs were introduced recently in dotnet/runtime#33042 and already used in several places e.g. in `Task`, `InterlockedBitVector32`, `SafeHandle`, `RegexCharClass`

It should emit `%reg = atomicrmw and (or)` in LLVM IR. (and unlike `Interlocked.Add` these API return old value, see dotnet/runtime#33102

Addresses dotnet/runtime#32239 for Mono.
EgorBo added a commit to mono/mono that referenced this pull request Mar 10, 2020
These APIs were introduced recently in dotnet/runtime#33042 and already used in several places e.g. in `Task`, `InterlockedBitVector32`, `SafeHandle`, `RegexCharClass`

It should emit `%reg = atomicrmw and (or)` in LLVM IR. (and unlike `Interlocked.Add` these API return old value, see dotnet/runtime#33102

Addresses dotnet/runtime#32239 for Mono.

Co-authored-by: EgorBo <EgorBo@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants