KEMBAR78
[mono][mini] Interlocked.CompareExchange and Interlocked.Exchange intrinsics for small types and enums by lambdageek · Pull Request #106660 · dotnet/runtime · GitHub
Skip to content

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
@lambdageek

This comment was marked as outdated.

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
Contributor

@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

</Target>

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

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
Copy link
Member Author

/backport to release/9.0

@github-actions
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