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][mini] Interlocked.CompareExchange and Interlocked.Exchange intrinsics for small types and enums #106660

Merged
merged 26 commits into from
Aug 23, 2024

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 19, 2024

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


TODO:

  • CompareExchange(u8/i8)
  • CompareExchange(u16/i16)
  • Exchange(u8/i8)
  • Exchange(u16/i16)
  • merge @kg jiterp branch into PR
  • interp+jiterp Interlocked.Exchange(int) intrinsics

Related to #105335
Related to #93488

@lambdageek lambdageek changed the title [mono][mini][arm64] Interlocked.CompareExchange for byte/sbyte [mono][mini] Interlocked.CompareExchange for byte/sbyte Aug 20, 2024
@lambdageek lambdageek marked this pull request as ready for review August 20, 2024 19:54
@lambdageek lambdageek changed the title [mono][mini] Interlocked.CompareExchange for byte/sbyte [mono][mini] Interlocked.CompareExchange intrinsics for small types and enums Aug 20, 2024
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

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

Interp parts look correct to me.

@lewing
Copy link
Member

lewing commented Aug 23, 2024

@kg do you want to do the jiterp work here or in a follow up?

@kg
Copy link
Member

kg commented Aug 23, 2024

Jiterp implementation should be done already, I pushed it into the PR.

@lewing
Copy link
Member

lewing commented Aug 23, 2024

Jiterp implementation should be done already, I pushed it into the PR.

for all i/u 4 variants?

@kg
Copy link
Member

kg commented Aug 23, 2024

Jiterp implementation should be done already, I pushed it into the PR.

for all i/u 4 variants?

i/u 4 variants aren't in this PR, I think? They're on the todo list.

Copy link
Member

@jkurdek jkurdek left a comment

Choose a reason for hiding this comment

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

arm changes look good to me

@@ -44,6 +45,10 @@
</MonoAOTCompiler>
</Target>

<ItemGroup>
<ProjectReference Include="$(RepoTasksDir)AotCompilerTask\MonoAOTCompiler.csproj" ReferenceOutputAssembly="false" Condition="'$(RunAOTCompilation)' == 'true'" AdditionalProperties="Configuration=Debug"/>
Copy link
Member

Choose a reason for hiding this comment

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

For my education, why those changes were necessary? I thought we could already run the mono sample with FullAOT

Copy link
Member Author

Choose a reason for hiding this comment

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

./build.sh mono+libs+tasks on OSX it doesn't bulid the AOT compiler task anymore because someone added a condition to tasks.proj to skip it on non-mobile platforms. I think the long-term direction for tasks.proj is to get rid of it and add proper project references to all the projects that need tasks

@lambdageek
Copy link
Member Author

lambdageek commented Aug 23, 2024

Jiterp implementation should be done already, I pushed it into the PR.

for all i/u 4 variants?

I'm going to add i4 for the interpreter and jiterp today Done

Comment on lines +712 to +721
OPDEF(MINT_MONO_EXCHANGE_U1, "mono_interlocked.xchg.u1", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_EXCHANGE_I1, "mono_interlocked.xchg.i1", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_EXCHANGE_U2, "mono_interlocked.xchg.u2", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_EXCHANGE_I2, "mono_interlocked.xchg.i2", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_EXCHANGE_I4, "mono_interlocked.xchg.i4", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_EXCHANGE_I8, "mono_interlocked.xchg.i8", 4, 1, 2, MintOpNoArgs)
OPDEF(MINT_MONO_CMPXCHG_U1, "mono_interlocked.cmpxchg.u1", 5, 1, 3, MintOpNoArgs)
OPDEF(MINT_MONO_CMPXCHG_I1, "mono_interlocked.cmpxchg.i1", 5, 1, 3, MintOpNoArgs)
OPDEF(MINT_MONO_CMPXCHG_U2, "mono_interlocked.cmpxchg.u2", 5, 1, 3, MintOpNoArgs)
OPDEF(MINT_MONO_CMPXCHG_I2, "mono_interlocked.cmpxchg.i2", 5, 1, 3, MintOpNoArgs)
Copy link
Member Author

Choose a reason for hiding this comment

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

@BrzVlad @kg instead of having a ton of different opcodes, we could have a single MINT_MONO_EXCHANGE_INT (and MINT_MONO_CMPXCHG_INT) opcode with a isSigned << 4 | size byte argument. That would make the big opcode switch in interp.c a little smaller, but make the code a little more complicated. Any opinions about which way is better?

@lambdageek lambdageek merged commit 86b58ee into dotnet:main Aug 23, 2024
152 of 158 checks passed
@lambdageek
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10531732073

@stephentoub
Copy link
Member

Thank you, @lamdageek.

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.

6 participants