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

[Mono] Implement small types atomic intrinsics for CompareExchange and Exchange #93488

Closed
Tracked by #43051
lambdageek opened this issue Oct 13, 2023 · 7 comments
Closed
Tracked by #43051

Comments

@lambdageek
Copy link
Member

lambdageek commented Oct 13, 2023

Update After #99011 Mono now uses the slower managed fallback implementations of these methods from src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs:

    public static byte CompareExchange(ref byte location1, byte value, byte comparand);
    public static sbyte CompareExchange(ref sbyte location1, sbyte value, sbyte comparand);
    public static short CompareExchange(ref short location1, short value, short comparand);
    public static ushort CompareExchange(ref ushort location1, ushort value, ushort comparand);

and

    public static byte Exchange(ref byte location1, byte value);
    public static sbyte Exchange(ref sbyte location1, sbyte value);
    public static short Exchange(ref short location1, short value);
    public static ushort Exchange(ref ushort location1, ushort value);

We should implement intrinsics for them in the JIT and interpreter, instead.


Original issue

After #92974 is merged, we will have icalls for CompareExchange and Exchange for byte, sbyte, shortandushort`

We should also implement these intrinsics directly in the JIT/AOT and interpreter

Part of #64658

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 13, 2023
@ghost
Copy link

ghost commented Oct 13, 2023

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

After #92974 is merged, we will have icalls for

   public static byte CompareExchange(ref byte location1, byte value, byte comparand) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static sbyte CompareExchange(ref sbyte location1, sbyte value, sbyte comparand) { throw null; }
        public static short CompareExchange(ref short location1, short value, short comparand) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static ushort CompareExchange(ref ushort location1, ushort value, ushort comparand) { throw null; }

and

      public static byte Exchange(ref byte location1, byte value) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static sbyte Exchange(ref sbyte location1, sbyte value) { throw null; }
        public static short Exchange(ref short location1, short value) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static ushort Exchange(ref ushort location1, ushort value) { throw null; }

We should also implement these intrinsics directly in the JIT/AOT and interpreter

Part of #64658

Author: lambdageek
Assignees: -
Labels:

area-System.Threading

Milestone: -

@ghost
Copy link

ghost commented Oct 13, 2023

Tagging subscribers to this area: @SamMonoRT, @fanyang-mono
See info in area-owners.md if you want to be subscribed.

Issue Details

After #92974 is merged, we will have icalls for

   public static byte CompareExchange(ref byte location1, byte value, byte comparand) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static sbyte CompareExchange(ref sbyte location1, sbyte value, sbyte comparand) { throw null; }
        public static short CompareExchange(ref short location1, short value, short comparand) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static ushort CompareExchange(ref ushort location1, ushort value, ushort comparand) { throw null; }

and

      public static byte Exchange(ref byte location1, byte value) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static sbyte Exchange(ref sbyte location1, sbyte value) { throw null; }
        public static short Exchange(ref short location1, short value) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public static ushort Exchange(ref ushort location1, ushort value) { throw null; }

We should also implement these intrinsics directly in the JIT/AOT and interpreter

Part of #64658

Author: lambdageek
Assignees: -
Labels:

area-System.Threading, untriaged, area-Codegen-Intrinsics-mono

Milestone: -

@lambdageek lambdageek removed area-System.Threading untriaged New issue has not been triaged by the area owner labels Oct 13, 2023
@lambdageek lambdageek added this to the 9.0.0 milestone Oct 13, 2023
@lambdageek
Copy link
Member Author

/cc @SamMonoRT

@fanyang-mono fanyang-mono changed the title [Mono] Implement small types atomic intrinsics [Mono] Implement small types atomic intrinsics for CompareExchange and Exchange Oct 13, 2023
@lambdageek
Copy link
Member Author

/cc @steveisok

@lambdageek
Copy link
Member Author

After #104558 this is now important (particular to intrinsify Interlocked.Exchange<bool> and enums) - see #105335

@lambdageek
Copy link
Member Author

There's a detailed explanation of what happend in #104558 (comment)

@lambdageek lambdageek self-assigned this Aug 13, 2024
lambdageek added a commit that referenced this issue Aug 23, 2024
…rinsics for small types and enums (#106660)

Only arm64, amd64 and WebAssembly. Interpreter, JIT and LLVM AOT. No x86 or arm32.

The complication for the 8- and 16-bit operands is that the .NET operand stack is 32-bit. so we have to zero- or sign-extend the inputs and output.

I ended up following the same approach as clang: zero-extend "expected" value and then sign-extend the return value of the CAS.  One nice consequence is that the LLVM support is essentially free.

---

Also this PR makes the intrinsic apply to types that have byte/int/long as the underlying type (bool, enums) not just when the type is literally in the signature

---

Also implement interp and jiterp intrinsic for Exchange(int) - this was not available previously.

---


Related to #105335
Related to #93488

* [mono][mini][arm64] Interlocked.CompareExchange for byte/sbyte

* [sample] fixup HelloWorld to work with full aot

* [mini][llvm] support OP_ATOMIC_CAS_U1
* [mini][amd64] support OP_ATOMIC_CAS_U1
* [mini][wasm] support OP_ATOMIC_CAS_U1 in LLVM AOT

* make intrinsic must-expand on arm64,amd64,wasm on mono

* [interp] MINT_MONO_CMPXCHG_U1

   also add mono_atomic_cas_u8 utility function

* [mini] CompareExchange(i16/u16)

* [mono] must expand CompareExchange(i16/u16)

* [interp] Interlocked.CompareExchange(u16/i16)

* [mini] zext unsigned CAS results for small types

* [interp] signed small CAS ops

* [amd64] fixup 8- and 16-bit cmpxchg encoding

* fixup u16 win32 atomic

* Interlocked.Exchange u1/i1/u2/i2 interp,llvm,amd64,arm64

* [amd64] give u2 CMPXCHG and XCHG one more byte

   for the 16-bit addressing prefix

* If jiterpreter is engaged before the thread is fully initialized, just fail to allocate a table index and generate a warning. 
   This shouldn't happen in prod anyway

* Implement cmpxchg atomics natively in jiterpreter

* Remove unnecessary jiterp cas helpers

* Do cmpxchg result fixups as needed

* Add runtime option for jiterpreter atomics

* Implement atomic exchanges in the jiterpreter

* Interlocked.Exchange(int) for interp and jiterp

---------

Co-authored-by: Katelyn Gadd <kg@luminance.org>
@lambdageek
Copy link
Member Author

fixed in .NET 10 by #106660, and for .NET 9 by #106897

@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant