[Support/rpmalloc] Updated fake atomics with Interlocked functions #148303
Conversation
|
@llvm/pr-subscribers-llvm-support Author: Dmitry Vasilyev (slydiman) ChangesMost atomic functions used Interlocked functions in case of MSVC (since MSVC does not do C11 yet). This PR fixes #146205. Full diff: https://github.com/llvm/llvm-project/pull/148303.diff 1 Files Affected:
diff --git a/llvm/lib/Support/rpmalloc/rpmalloc.c b/llvm/lib/Support/rpmalloc/rpmalloc.c
index a06d3cdb5b52e..7e30922cbabc6 100644
--- a/llvm/lib/Support/rpmalloc/rpmalloc.c
+++ b/llvm/lib/Support/rpmalloc/rpmalloc.c
@@ -275,9 +275,11 @@ typedef volatile long atomic32_t;
typedef volatile long long atomic64_t;
typedef volatile void *atomicptr_t;
-static FORCEINLINE int32_t atomic_load32(atomic32_t *src) { return *src; }
+static FORCEINLINE int32_t atomic_load32(atomic32_t *src) {
+ return (int32_t)InterlockedOr(src, 0);
+}
static FORCEINLINE void atomic_store32(atomic32_t *dst, int32_t val) {
- *dst = val;
+ InterlockedExchange(dst, val);
}
static FORCEINLINE int32_t atomic_incr32(atomic32_t *val) {
return (int32_t)InterlockedIncrement(val);
@@ -293,20 +295,22 @@ static FORCEINLINE int atomic_cas32_acquire(atomic32_t *dst, int32_t val,
return (InterlockedCompareExchange(dst, val, ref) == ref) ? 1 : 0;
}
static FORCEINLINE void atomic_store32_release(atomic32_t *dst, int32_t val) {
- *dst = val;
+ InterlockedExchange(dst, val);
+}
+static FORCEINLINE int64_t atomic_load64(atomic64_t *src) {
+ return (int64_t)InterlockedOr64(src, 0);
}
-static FORCEINLINE int64_t atomic_load64(atomic64_t *src) { return *src; }
static FORCEINLINE int64_t atomic_add64(atomic64_t *val, int64_t add) {
return (int64_t)InterlockedExchangeAdd64(val, add) + add;
}
static FORCEINLINE void *atomic_load_ptr(atomicptr_t *src) {
- return (void *)*src;
+ return InterlockedCompareExchangePointer(src, 0, 0);
}
static FORCEINLINE void atomic_store_ptr(atomicptr_t *dst, void *val) {
- *dst = val;
+ InterlockedExchangePointer(dst, val);
}
static FORCEINLINE void atomic_store_ptr_release(atomicptr_t *dst, void *val) {
- *dst = val;
+ InterlockedExchangePointer(dst, val);
}
static FORCEINLINE void *atomic_exchange_ptr_acquire(atomicptr_t *dst,
void *val) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Most atomic functions used Interlocked functions in case of MSVC (since MSVC does not do C11 yet). But few load/store functions are dummy. Use Interlocked functions for these atomics to ensure they are thread-safe. This PR fixes llvm#146205.
slydiman
force-pushed
the
fix-rpmalloc-intrinsics
branch
from
0d16751 to
f300dcb
Compare
|
I'd like to see this merged upstream rpmalloc before merging it here. @slydiman Can you please send a PR there first? https://github.com/mjansson/rpmalloc/pulls The point being, like for BLAKE3, I think we should keep in sync with upstream rpmalloc and only make integrations here into LLVM. You might also want to experiment with the compiler flag |
The usage of Interlocked* functions is the workaround since MSVC does not do C11.
|
|
We do require VS 2019 version 16.8 minimally in LLVM, please see 28bba0d However that is not enough afaik for C11 stdatomics, it needs VS 2022 version 17.5 minimally: https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ I am fine with the Interlocked versions, but again, I'd like to keep this in sync with upstream rpmalloc, and not diverge with custom changes. We could:
Are you able to do that first step? |
|
All
rpmalloc upstream does not have this problem any more, thus the fix is not needed there. |
Actually the rpmalloc fork in LLVM is version 1.4.5 verbatim. What you might looking at is the develop branch which indeed had the "Atomic access abstraction" part removed from the code in this commit mjansson/rpmalloc@ed40516.
@mjansson Is there a possibility that you release a new rpmalloc version soon with the contents of the develop branch? There haven't been any releases for the past year. We're trying to see what avenues we should take with the change proposed in this PR. Ideally I'd like not to diverge from your repo. One other option would be to integrate into LLVM from develop at head (if you think that should be stable enough). |
Most atomic functions used Interlocked functions in case of MSVC (since MSVC does not do C11 yet).
But few load/store functions are dummy.
Use Interlocked functions for these atomics to ensure they are thread-safe.
This PR fixes #146205.