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

[NativeAOT] Move Interlocked null checks to managed, RhpLockCmpXchg64 to C #100021

Merged
merged 10 commits into from
Mar 22, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Mar 20, 2024

  • Move RhpLockCmpXchg[32/64] implementation to C
  • Move null checks from RhpLockCmpXchg[8/16/32/64]/RhpCheckedLockCmpXchg/RhpCheckedXchg to managed code

Ref: #99688 (comment)
Ref: #96916 (similar thing done for CoreCLR)

…ockCmpXchg64/RhpCheckedLockCmpXchg/RhpCheckedXchg to managed code
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2024
@@ -320,7 +315,6 @@ NotInHeap
b DoCardsCmpXchg
CmpXchgRetry
;; Check location value is what we expect.
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation2
ldaxr x10, [x0]
Copy link
Member Author

Choose a reason for hiding this comment

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

The RhpCheckedLockCmpXchgAVLocation2 and RhpCheckedXchgAVLocation2 checks are weird. They seem to be present for the same location that was already null checked and unnecessary.

@filipnavara filipnavara requested a review from jkotas March 20, 2024 15:04
@filipnavara
Copy link
Member Author

Kept as draft, needs to be run through CI.

bne LOCAL_LABEL(CmpXchg64Retry)
LOCAL_LABEL(CmpXchg64Exit):
mov r0, r6
dmb
Copy link
Member

Choose a reason for hiding this comment

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

The native AOT PAL implementation of InterlockedCompareExchange64 seems to be missing this barrier (PAL_InterlockedOperationBarrier in CoreCLR PAL).

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it but I noticed that PAL_InterlockedOperationBarrier is not enabled for arm32. That seems odd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added it but I noticed that PAL_InterlockedOperationBarrier is not enabled for arm32. That seems odd.

IIRC the native compilers already emit a barrier of their own there so it'd do two barriers (so I guess it's kinda relying on an implementation detail).

Copy link
Member Author

Choose a reason for hiding this comment

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

the native compilers already emit a barrier of their own

At very least ARM gcc and ARM clang does, so that explains why it wasn't a problem for ARM32 on CoreCLR.

Copy link
Member Author

Choose a reason for hiding this comment

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

ARM64: clang generates barrier at the end (should still be fine); gcc doesn't generate a barrier

@@ -5,28 +5,51 @@

#include <errno.h>

FORCEINLINE void PalInterlockedOperationBarrier()
{
#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT)) || defined(HOST_ARM) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT)) || defined(HOST_ARM) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64)
#if (defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT) && !defined(__clang__)) || defined(HOST_LOONGARCH64) || defined(HOST_RISCV64)

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK C++ code is not supposed to have compiler specific defines in the runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, other code in the same file already uses __clang__.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, also _MSC_VER and __llvm__. (I think we can replace all __llvm__ ones to __clang__ for consistency?)

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK C++ code is not supposed to have compiler specific defines in the runtime

I do not see a problem with this ifdef here.

@jkotas
Copy link
Member

jkotas commented Mar 22, 2024

@filipnavara Could you please resolve the conflict?

@MichalPetryka
Copy link
Contributor

I did it myself cause I have push perms to filips fork.

@MichalPetryka
Copy link
Contributor

  • Move RhpLockCmpXchg64 implementation to C

Why leave the 32bit one in ASM btw?

@jkotas
Copy link
Member

jkotas commented Mar 22, 2024

Why leave the 32bit one in ASM btw?

Do you mean the one for Arm? Yes, it would be nice to move it to C as well. Filip have not done it earlier to reduce conflicts with your other PR.

@MichalPetryka
Copy link
Contributor

Do you mean the one for Arm?

Only until #97792 is merged.

Filip have not done it earlier to reduce conflicts with your other PR.

I don't think we'd even have any conflicts there since they'd both just remove it.

@filipnavara
Copy link
Member Author

Yes, it would be nice to move it to C as well.

Done.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkotas jkotas merged commit 2820a1e into dotnet:main Mar 22, 2024
88 of 92 checks passed
@filipnavara filipnavara deleted the interlocked-null-check branch March 22, 2024 07:27
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants