Consider making Interlocked.And and friends into JIT intrinsics

[stephentoub]

From #32216 (comment)

#32216 adds new methods like Interlocked.And. Some of these could be done as JIT intrinsics and replaced by better implementations/instructions in some cases, like lock and on platforms that have it.

cc: @tannergooding

category:cq
theme:jit-intrinsics
skill-level:intermediate
cost:medium
impact:medium

[BruceForstall]

@AndyAyersMS

[GrabYourPitchforks]

For context, it turns out the JIT already optimizes this for the existing Interlocked.Add and similar APIs.

using System;
using System.Threading;

public class C {
    public static int M(ref int i) {
        return Interlocked.Add(ref i, 1);
    }
    
    public static void N(ref int i) {
        Interlocked.Add(ref i, 1);
    }
}

; C.M(Int32 ByRef)
    L0000: mov eax, 0x1
    L0005: lock xadd [rcx], eax
    L0009: inc eax
    L000b: ret

; C.N(Int32 ByRef)
    L0000: lock add dword [rcx], 0x1
    L0004: ret

[stephentoub]

Nice. This can be closed then, right?

Oh.. you said Add, not And. Those are way too closely named :-) I was very impressed it was able to reverse the pattern in Interlocked.And :-)

[AndyAyersMS]

These should be done via new-style intrinsics. We may also want to convert some of the existing intrinsified interlocked ops over to this mechanism as well.

[AndyAyersMS]

@Stoub we're triaging proposed 5.0 codegen work -- any thoughts on the priority of this issue?

[stephentoub]

From my perspective, "nice to have", but I expect there's other higher priority JIT work that'll be more impactful. These APIs are brand new.

[AndyAyersMS]

@GrabYourPitchforks can you make a case for addressing this in 5.0?

[GrabYourPitchforks]

I agree with Steve's assessment. Optimizing these APIs might save a few cycles in a handful of places, such as below.

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Lines 739 to 740 in ef2ea8a

	// Atomically clear the TASK_STATE_WAIT_COMPLETION_NOTIFICATION bit
	Interlocked.And(ref m_stateFlags, ~TASK_STATE_WAIT_COMPLETION_NOTIFICATION);

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/SafeHandle.cs

Lines 104 to 105 in ef2ea8a

	// Set closed state (low order bit of the _state field).
	Interlocked.Or(ref _state, StateBits.Closed);

These are hot code paths, but not so hot that a few extra cycles are going to kill us. :) If you have other more pressing matters, so be it.

[AndyAyersMS]

Ok, I'm going to move this to future.

[JulieLeeMSFT]

@echesakovMSFT Possible candidate for .NET 6.

[EgorBo]

this is a bit tricky for the cases when return value of And/Or is used 🙂

[GrabYourPitchforks]

Wouldn't it be able to use the same logic Interlocked.Add and friends use? See my earlier comment at #32239 (comment).

[EgorBo]

@GrabYourPitchforks here is what LLVM emits for us (mono-llvm): https://godbolt.org/z/vbsYz3

Quoting LLVM:

      // There are two different ways of expanding RMW instructions:
      // - into a load if it is idempotent
      // - into a Cmpxchg/LL-SC loop otherwise

Or/And aren't idempotent, Add is.

[EgorBo]

better demo: https://godbolt.org/z/cMj5Tx

[echesakov]

When using Armv8.1 instructions the clang/llvm generated code seems much better than its Armv8.0 counterpart https://godbolt.org/z/Yvjcqb and there is no loops required.

Armv8.0

InterlockedAdd(int*, int):                  // @InterlockedAdd(int*, int)
.LBB0_1:                                // =>This Inner Loop Header: Depth=1
        ldaxr   w8, [x0]
        add     w8, w8, w1
        stlxr   w9, w8, [x0]
        cbnz    w9, .LBB0_1
        mov     w0, w8
        ret
InterlockedAnd(int*, int):                  // @InterlockedAnd(int*, int)
.LBB1_1:                                // =>This Inner Loop Header: Depth=1
        ldaxr   w8, [x0]
        and     w8, w8, w1
        stlxr   w9, w8, [x0]
        cbnz    w9, .LBB1_1
        mov     w0, w8
        ret
InterlockedOr(int*, int):                   // @InterlockedOr(int*, int)
.LBB2_1:                                // =>This Inner Loop Header: Depth=1
        ldaxr   w8, [x0]
        orr     w8, w8, w1
        stlxr   w9, w8, [x0]
        cbnz    w9, .LBB2_1
        mov     w0, w8
        ret

Armv8.1

InterlockedAdd(int*, int):                  // @InterlockedAdd(int*, int)
        ldaddal w1, w8, [x0]
        add     w0, w8, w1
        ret
InterlockedAnd(int*, int):                  // @InterlockedAnd(int*, int)
        mvn     w8, w1
        ldclral w8, w8, [x0]
        and     w0, w8, w1
        ret
InterlockedOr(int*, int):                   // @InterlockedOr(int*, int)
        ldsetal w1, w8, [x0]
        orr     w0, w8, w1
        ret

[EgorBo]

@echesakovMSFT Do you mind if I try to implement Or and And for arm ? (with the atomics)
In case if you haven't started it yet of course. I just want to do something for Arm back-end 🙂

[echesakov]

@EgorBo I haven't started working on this. If you want to work on this, feel free to do so and un-assign me.

[EgorBo]

Reopening since #46253 only handled them for arm64

[EgorBo]

Moving to Future, it's not easy to optimize it for x86 unfortunately because we only can optimize cases where return value of these methods won't be used and we can't check that fact at the importer phase easily.

[tannergooding]

Moving to Future, it's not easy to optimize it for x86 unfortunately because we only can optimize cases where return value of these methods won't be used and we can't check that fact at the importer phase easily.

This is actually a decently broad issue with intrinsics. There are many cases where we want to optimistically create an intrinsic tree node but where we won't know if it can actually be an intrinsic or call until much later.
With hardware intrinsics, this leads to codegen issues for things that are detected to be constants after importation, for example.

Some of the older intrinsics (GT_INTRINSIC) have support in rationalizer to be rewritting as user calls: RewriteIntrinsicAsUserCall
I had taken a look at enabling this more broadly for cases like GT_HWINTRINSIC so we could fix cases like this but ran into issues due to the non-primitive inputs/returns (and these were out of my depth to resolve at the time): #11062 (comment)

Given that these interlocked methods only deal with primitives, its possible that it won't hit the same problems.

[EgorBo]

Some of the older intrinsics (GT_INTRINSIC) have support in rationalizer to be rewritting as user calls: RewriteIntrinsicAsUserCall

@tannergooding the problem that if we re-write GT_XAND to a call like that - we'll regress the current case when we inline it as complex expression (contains a loop) as is. see https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgBUALWbAEwEsAdgHMA3DRrEAzE1IMAwgwDeNBqoYq1G1VKYoGbGLgwAKWADMGgjA2zk0lgdeykAlFqXu1DAPTeGAQQEeS1wHABtBGGCOGFhPNQBJR1iwiDAAayiWQJ5TGAtbe2cXMWovAF8acqA==

Perhaps, it's not a big deal ("call" overhead for the case where return value is used and super fast implementation for cases where it's not -- e.g. all BCL usages (there are just a few of them) don't care about return value.