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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Mar 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1d81cec
Move RhpLockCmpXchg64 implementation to C; move null checks from RhpL…
filipnavara Mar 20, 2024
3b2e99e
Removed #ifs per feedback
filipnavara Mar 20, 2024
fb6ec76
Fix cut & paste error
filipnavara Mar 20, 2024
9447a82
Update RhpLockCmpXchg[8/16/32] too
filipnavara Mar 20, 2024
1cb0367
Fix build
filipnavara Mar 20, 2024
a414651
Add PalInterlockedOperationBarrier
filipnavara Mar 21, 2024
6a96d20
PR feedback
filipnavara Mar 21, 2024
cd2d77d
PR feedback
filipnavara Mar 21, 2024
0c166b6
Merge branch 'main' into interlocked-null-check
MichalPetryka Mar 22, 2024
78b9258
Move RhpLockCmpXchg32 to C
filipnavara Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/nativeaot/Runtime/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,11 @@ if (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32)
)
endif (CLR_CMAKE_TARGET_ARCH_AMD64 AND CLR_CMAKE_TARGET_WIN32)

if (NOT (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64))
if (CLR_CMAKE_TARGET_ARCH_ARM)
list(APPEND RUNTIME_SOURCES_ARCH_ASM
${ARCH_SOURCES_DIR}/Interlocked.${ASM_SUFFIX}
)
endif (NOT (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64))
endif (CLR_CMAKE_TARGET_ARCH_ARM)
jkotas marked this conversation as resolved.
Show resolved Hide resolved

list(APPEND RUNTIME_SOURCES_ARCH_ASM
${ARCH_SOURCES_DIR}/AllocFast.${ASM_SUFFIX}
Expand Down
14 changes: 1 addition & 13 deletions src/coreclr/nativeaot/Runtime/EHHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,13 +326,10 @@ EXTERN_C CODE_LOCATION RhpCheckedAssignRefESIAVLocation;
EXTERN_C CODE_LOCATION RhpCheckedAssignRefEDIAVLocation;
EXTERN_C CODE_LOCATION RhpCheckedAssignRefEBPAVLocation;
#endif
EXTERN_C CODE_LOCATION RhpCheckedLockCmpXchgAVLocation;
EXTERN_C CODE_LOCATION RhpCheckedXchgAVLocation;
#if !defined(HOST_AMD64) && !defined(HOST_ARM64)
EXTERN_C CODE_LOCATION RhpLockCmpXchg8AVLocation;
EXTERN_C CODE_LOCATION RhpLockCmpXchg16AVLocation;
EXTERN_C CODE_LOCATION RhpLockCmpXchg32AVLocation;
EXTERN_C CODE_LOCATION RhpLockCmpXchg64AVLocation;
#endif
EXTERN_C CODE_LOCATION RhpByRefAssignRefAVLocation1;

Expand Down Expand Up @@ -368,23 +365,14 @@ static bool InWriteBarrierHelper(uintptr_t faultingIP)
(uintptr_t)&RhpCheckedAssignRefEDIAVLocation,
(uintptr_t)&RhpCheckedAssignRefEBPAVLocation,
#endif
(uintptr_t)&RhpCheckedLockCmpXchgAVLocation,
(uintptr_t)&RhpCheckedXchgAVLocation,
#if !defined(HOST_AMD64) && !defined(HOST_ARM64)
#if !defined(HOST_X86)
#if defined(HOST_ARM)
(uintptr_t)&RhpLockCmpXchg8AVLocation,
(uintptr_t)&RhpLockCmpXchg16AVLocation,
(uintptr_t)&RhpLockCmpXchg32AVLocation,
#endif
(uintptr_t)&RhpLockCmpXchg64AVLocation,
#endif
(uintptr_t)&RhpByRefAssignRefAVLocation1,
#if !defined(HOST_ARM64)
(uintptr_t)&RhpByRefAssignRefAVLocation2,
#endif
#if defined(HOST_ARM64) && !defined(LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT)
(uintptr_t)&RhpCheckedLockCmpXchgAVLocation2,
(uintptr_t)&RhpCheckedXchgAVLocation2,
#endif
};

Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/nativeaot/Runtime/MiscHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,3 +416,9 @@ EXTERN_C void QCALLTYPE RhCpuIdEx(int* cpuInfo, int functionId, int subFunctionI
__cpuidex(cpuInfo, functionId, subFunctionId);
}
#endif

FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand)
{
return PalInterlockedCompareExchange64(location, value, comparand);
}
FCIMPLEND
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,28 +221,20 @@ LEAF_END RhpCheckedAssignRef\EXPORT_REG_NAME, _TEXT
// just one write barrier that assumes the input register is RSI.
DEFINE_CHECKED_WRITE_BARRIER RSI, ESI

// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation
// - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT
mov rax, rdx
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation
lock cmpxchg [rdi], rsi
jne LOCAL_LABEL(RhpCheckedLockCmpXchg_NoBarrierRequired_RSI)

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, RSI

LEAF_END RhpCheckedLockCmpXchg, _TEXT

// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation
// - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
LEAF_ENTRY RhpCheckedXchg, _TEXT

// Setup rax with the new object for the exchange, that way it will automatically hold the correct result
// afterwards and we can leave rdx unaltered ready for the GC write barrier below.
mov rax, rsi
ALTERNATE_ENTRY RhpCheckedXchgAVLocation
xchg [rdi], rax

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, RSI
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/amd64/WriteBarriers.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,28 +237,20 @@ endm
;; just one write barrier that assumes the input register is RDX.
DEFINE_CHECKED_WRITE_BARRIER RDX, EDX

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation
;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT
mov rax, r8
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation
lock cmpxchg [rcx], rdx
jne RhpCheckedLockCmpXchg_NoBarrierRequired_RDX

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, RDX

LEAF_END RhpCheckedLockCmpXchg, _TEXT

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation
;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
LEAF_ENTRY RhpCheckedXchg, _TEXT

;; Setup rax with the new object for the exchange, that way it will automatically hold the correct result
;; afterwards and we can leave rdx unaltered ready for the GC write barrier below.
mov rax, rdx
ALTERNATE_ENTRY RhpCheckedXchgAVLocation
xchg [rcx], rax

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, RDX
Expand Down
27 changes: 0 additions & 27 deletions src/coreclr/nativeaot/Runtime/arm/Interlocked.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,30 +73,3 @@ LOCAL_LABEL(CmpXchg32Exit):
dmb
bx lr
LEAF_END RhpLockCmpXchg32, _TEXT

// WARNING: Code in EHHelpers.cpp makes assumptions about this helper, in particular:
// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpLockCmpXchg64AVLocation
// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address
// r0 = destination address
// {r2,r3} = value
// sp[0+8] = comparand
LEAF_ENTRY RhpLockCmpXchg64, _TEXT
GLOBAL_LABEL RhpLockCmpXchg64AVLocation
ldr r12, [r0] // dummy read for null check
PROLOG_PUSH "{r4-r6,lr}"
dmb
ldrd r4, r5, [sp,#0x10]
LOCAL_LABEL(CmpXchg64Retry):
ldrexd r6, r1, [r0]
cmp r6, r4
bne LOCAL_LABEL(CmpXchg64Exit)
cmp r1, r5
bne LOCAL_LABEL(CmpXchg64Exit)
strexd r12, r2, r3, [r0]
cmp r12, #0
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

EPILOG_POP "{r4-r6,pc}"
LEAF_END RhpLockCmpXchg64, _TEXT
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/arm/WriteBarriers.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,6 @@ LEAF_END RhpCheckedAssignRef\EXPORT_REG_NAME, _TEXT
// just one write barrier that assumes the input register is RSI.
DEFINE_CHECKED_WRITE_BARRIER r1, r1

// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation
// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address
// r0 = destination address
// r1 = value
// r2 = comparand
Expand All @@ -261,7 +258,6 @@ LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT
// barrier must occur before the object reference update, so we have to do it unconditionally even
// though the update may fail below.
dmb
GLOBAL_LABEL RhpCheckedLockCmpXchgAVLocation
LOCAL_LABEL(RhpCheckedLockCmpXchgRetry):
ldrex r3, [r0]
cmp r2, r3
Expand All @@ -277,16 +273,12 @@ LOCAL_LABEL(RhpCheckedLockCmpXchgRetry):
bx lr
LEAF_END RhpCheckedLockCmpXchg, _TEXT

// WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
// - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedXchgAVLocation
// - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address
// r0 = destination address
// r1 = value
LEAF_ENTRY RhpCheckedXchg, _TEXT
// To implement our chosen memory model for ARM we insert a memory barrier at GC write barriers. This
// barrier must occur before the object reference update.
dmb
GLOBAL_LABEL RhpCheckedXchgAVLocation
LOCAL_LABEL(RhpCheckedXchgRetry):
ldrex r2, [r0]
strex r3, r1, [r0]
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,6 @@ LEAF_END RhpAssignRef, _TEXT
#endif

mov x10, x2
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation
casal x10, x1, [x0] // exchange
cmp x2, x10
bne LOCAL_LABEL(CmpXchgNoUpdate)
Expand All @@ -311,7 +310,6 @@ LEAF_END RhpAssignRef, _TEXT
b LOCAL_LABEL(DoCardsCmpXchg)
LOCAL_LABEL(CmpXchgRetry):
// Check location value is what we expect.
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation2
ldaxr x10, [x0]
cmp x10, x2
bne LOCAL_LABEL(CmpXchgNoUpdate)
Expand Down Expand Up @@ -365,14 +363,12 @@ LOCAL_LABEL(NoBarrierCmpXchg):
tbz w16, #ARM64_ATOMICS_FEATURE_FLAG_BIT, LOCAL_LABEL(ExchangeRetry)
#endif

ALTERNATE_ENTRY RhpCheckedXchgAVLocation
swpal x1, x10, [x0] // exchange

#ifndef LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT
b LOCAL_LABEL(DoCardsXchg)
LOCAL_LABEL(ExchangeRetry):
// Read the existing memory location.
ALTERNATE_ENTRY RhpCheckedXchgAVLocation2
ldaxr x10, [x0]

// Attempt to update with the new value.
Expand Down
12 changes: 0 additions & 12 deletions src/coreclr/nativeaot/Runtime/arm64/WriteBarriers.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,6 @@ NotInHeap
;; Interlocked operation helpers where the location is an objectref, thus requiring a GC write barrier upon
;; successful updates.

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at RhpCheckedLockCmpXchgAVLocation
;; - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address

;; RhpCheckedLockCmpXchg(Object** dest, Object* value, Object* comparand)
;;
;; Interlocked compare exchange on objectref.
Expand All @@ -311,7 +307,6 @@ NotInHeap
#endif

mov x10, x2
ALTERNATE_ENTRY RhpCheckedLockCmpXchgAVLocation
casal x10, x1, [x0] ;; exchange
cmp x2, x10
bne CmpXchgNoUpdate
Expand All @@ -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.

cmp x10, x2
bne CmpXchgNoUpdate
Expand Down Expand Up @@ -350,10 +344,6 @@ NoBarrierCmpXchg

LEAF_END RhpCheckedLockCmpXchg

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen within at RhpCheckedXchgAVLocation
;; - Function "UnwindSimpleHelperToCaller" assumes no registers were pushed and LR contains the return address

;; RhpCheckedXchg(Object** destination, Object* value)
;;
;; Interlocked exchange on objectref.
Expand All @@ -374,14 +364,12 @@ NoBarrierCmpXchg
tbz x16, #ARM64_ATOMICS_FEATURE_FLAG_BIT, ExchangeRetry
#endif

ALTERNATE_ENTRY RhpCheckedXchgAVLocation
swpal x1, x10, [x0] ;; exchange

#ifndef LSE_INSTRUCTIONS_ENABLED_BY_DEFAULT
b DoCardsXchg
ExchangeRetry
;; Read the existing memory location.
ALTERNATE_ENTRY RhpCheckedXchgAVLocation2
ldaxr x10, [x0]

;; Attempt to update with the new value.
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/nativeaot/Runtime/i386/Interlocked.S
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions src/coreclr/nativeaot/Runtime/i386/Interlocked.asm
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/i386/WriteBarriers.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,28 +237,20 @@ DEFINE_CHECKED_WRITE_BARRIER EDX, ESI
DEFINE_CHECKED_WRITE_BARRIER EDX, EDI
DEFINE_CHECKED_WRITE_BARRIER EDX, EBP

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at @RhpCheckedLockCmpXchgAVLocation@0
;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
FASTCALL_FUNC RhpCheckedLockCmpXchg, 12
mov eax, [esp+4]
ALTERNATE_ENTRY _RhpCheckedLockCmpXchgAVLocation
lock cmpxchg [ecx], edx
jne RhpCheckedLockCmpXchg_NoBarrierRequired_ECX_EDX

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedLockCmpXchg, ECX, EDX, ret 4

FASTCALL_ENDFUNC

;; WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular:
;; - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen at @RhpCheckedXchgAVLocation@0
;; - Function "UnwindSimpleHelperToCaller" assumes the stack contains just the pushed return address
FASTCALL_FUNC RhpCheckedXchg, 8

;; Setup eax with the new object for the exchange, that way it will automatically hold the correct result
;; afterwards and we can leave edx unaltered ready for the GC write barrier below.
mov eax, edx
ALTERNATE_ENTRY _RhpCheckedXchgAVLocation
xchg [ecx], eax

DEFINE_CHECKED_WRITE_BARRIER_CORE RhpCheckedXchg, ECX, EDX, ret
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/nativeaot/Runtime/portable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,6 @@ FCIMPLEND

FCIMPL3(Object *, RhpCheckedLockCmpXchg, Object ** location, Object * value, Object * comparand)
{
// @TODO: USE_PORTABLE_HELPERS - Null check
Object * ret = (Object *)PalInterlockedCompareExchangePointer((void * volatile *)location, value, comparand);
InlineCheckedWriteBarrier(location, value);
return ret;
Expand Down Expand Up @@ -411,13 +410,6 @@ FCIMPL3(int32_t, RhpLockCmpXchg32, int32_t * location, int32_t value, int32_t co
}
FCIMPLEND

FCIMPL3(int64_t, RhpLockCmpXchg64, int64_t * location, int64_t value, int64_t comparand)
{
// @TODO: USE_PORTABLE_HELPERS - Null check
return PalInterlockedCompareExchange64(location, value, comparand);
}
FCIMPLEND

FCIMPL0(void*, RhAllocateThunksMapping)
{
return NULL;
Expand Down
Loading
Loading