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

Use non locking atomic intrinsics #97527

Closed
wants to merge 6 commits into from
Closed

Use non locking atomic intrinsics #97527

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30bae52
Use non locking atomic intrinsics
MichalPetryka Jan 25, 2024
795a310
Update pal.h
MichalPetryka Jan 25, 2024
92b65fb
Move checking to CMake
MichalPetryka Jan 26, 2024
fa1d819
Update configure.cmake
MichalPetryka Jan 26, 2024
c3cf1c4
Update configure.cmake
MichalPetryka Jan 26, 2024
492ed0e
Make the code build
MichalPetryka Jan 26, 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
27 changes: 25 additions & 2 deletions src/coreclr/pal/inc/pal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3639,24 +3639,42 @@ Define_InterlockMethod(
CHAR,
InterlockedExchange8(IN OUT CHAR volatile *Target, CHAR Value),
InterlockedExchange8(Target, Value),
#ifdef __clang__
return __sync_swap(pDst, iValue);
#elif ATOMIC_CHAR_LOCK_FREE == 2
Copy link
Member

Choose a reason for hiding this comment

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

We cannot depend on defines like this in CoreCLR PAL headers. CoreCLR PAL headers cannot depend on system headers where they are defined.

Copy link
Contributor Author

@MichalPetryka MichalPetryka Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would moving the checks for those to the FCall definitions make them okay? Or would they be not accessible there either and need to be somehow wrapped with CMake?

Copy link
Member

@jkotas jkotas Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No. Nothing in CoreCLR outside the PAL implementation can depend on system headers.

One way to fix this is to use __sync_swap clang-specific intrinsic for clang, and implement it using __sync_val_compare_and_swap for non-clang. Like:

#ifdef __clang__

Define_InterlockMethod(
    LONGLONG,
    InterlockedExchange64(IN OUT LONGLONG volatile *Target, IN LONGLONG Value),
    InterlockedExchange64(Target, Value),
    __sync_swap(Target, Value)
)

#else

inline LONGLONG InterlockedExchange64(LONGLONG volatile * Target, LONGLONG Value)
{
    LONGLONG Old;
    do {
        Old = *Target;
    } while (__sync_val_compare_and_swap(Target, Old, Value) != Old);

    PAL_InterlockedOperationBarrier();

    return Old;
}

#endif

Copy link
Contributor Author

@MichalPetryka MichalPetryka Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

__sync_val_compare_and_swap isn't guaranteed to be lock free in GCC like we've discussed earlier (GCC docs say it's implemented with __atomic in fact) so that wouldn't help here.
Maybe the define test could just be done in the CMake files completely instead?

Copy link
Member

Choose a reason for hiding this comment

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

I understand that it is not guaranteed, but it is reasonable to assume that the implementation is lock-free on platforms that we care about.

How would you do that in CMake files?

clang is fine. I would like to avoid some complicated solution that is just for gcc. As I have said earlier, we do not have any testing for gcc-based builds, so there is no way to validate that it actually works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you do that in CMake files?

Pushed a commit, a part of it is commented out cause I want to verify on the CI.

As I have said earlier, we do not have any testing for gcc-based builds, so there is no way to validate that it actually works.

I've seen that some Linux distros ship GCC built dotnet in their repositories so I'd prefer to guarantee its working for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Seems working I think.

__atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL)
#else
#error Lock free atomic int8 support is required.
#endif
)

Define_InterlockMethod(
SHORT,
InterlockedExchange16(IN OUT SHORT volatile *Target, SHORT Value),
InterlockedExchange16(Target, Value),
#ifdef __clang__
return __sync_swap(pDst, iValue);
#elif ATOMIC_SHORT_LOCK_FREE == 2
__atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL)
#else
#error Lock free atomic int16 support is required.
#endif
)

Define_InterlockMethod(
LONG,
InterlockedExchange(IN OUT LONG volatile *Target, LONG Value),
InterlockedExchange(Target, Value),
#ifdef __clang__
return __sync_swap(pDst, iValue);
#elif ATOMIC_INT_LOCK_FREE == 2
__atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL)
#else
#error Lock free atomic int32 support is required.
#endif
)

#if defined(HOST_X86)
#if defined(HOST_X86) && (defined(__clang__) || ATOMIC_LLONG_LOCK_FREE == 2)

// 64-bit __atomic_exchange_n is not expanded as a compiler intrinsic on Linux x86.
// Use inline implementation instead.
Expand All @@ -3678,12 +3696,17 @@ Define_InterlockMethod(
LONGLONG,
InterlockedExchange64(IN OUT LONGLONG volatile *Target, IN LONGLONG Value),
InterlockedExchange64(Target, Value),
#ifdef __clang__
return __sync_swap(pDst, iValue);
#elif ATOMIC_LLONG_LOCK_FREE == 2
__atomic_exchange_n(Target, Value, __ATOMIC_ACQ_REL)
#else
#error Lock free atomic int64 support is required.
#endif
)

#endif


/*++
Function:
InterlockedCompareExchange
Expand Down
Loading