Copilot commented on code in PR #3355:
URL: https://github.com/apache/brpc/pull/3355#discussion_r3466672023
##########
src/bthread/task_group.cpp:
##########
@@ -85,37 +85,100 @@ BAIDU_VOLATILE_THREAD_LOCAL(void*, tls_unique_user_ptr,
NULL);
const TaskStatistics EMPTY_STAT = { 0, 0, 0 };
AtomicInteger128::Value AtomicInteger128::load() const {
-#if __x86_64__ || __ARM_NEON
- // Supress compiler warning.
- (void)_mutex;
-#endif // __x86_64__ || __ARM_NEON
-
-#if __x86_64__ || __ARM_NEON
#ifdef __x86_64__
+ (void)_mutex;
+ (void)_seq;
__m128i value = _mm_load_si128(reinterpret_cast<const __m128i*>(&_value));
-#else // __ARM_NEON
+ return {value[0], value[1]};
+#elif defined(__ARM_NEON)
+ (void)_mutex;
+ (void)_seq;
int64x2_t value = vld1q_s64(reinterpret_cast<const int64_t*>(&_value));
-#endif // __x86_64__
return {value[0], value[1]};
-#else // __x86_64__ || __ARM_NEON
- // RISC-V and other architectures use mutex fallback
+#elif defined(__riscv)
+ (void)_mutex;
+ // RISC-V: Seqlock-based atomic 128-bit load.
+ int64_t v1, v2;
+ uint64_t seq0, seq1;
+ do {
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ : "=r"(seq0)
+ : "m"(_seq)
+ : "memory"
+ );
+ if (seq0 & 1) continue;
+ __asm__ volatile("fence r, rw\n\t" ::: "memory");
+ __asm__ volatile(
+ "ld %0, %2\n\t"
+ "ld %1, %3\n\t"
+ : "=r"(v1), "=r"(v2)
+ : "m"(_value.v1), "m"(_value.v2)
+ : "memory"
+ );
+ __asm__ volatile("fence r, rw\n\t" ::: "memory");
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ : "=r"(seq1)
+ : "m"(_seq)
+ : "memory"
+ );
+ } while (seq0 != seq1);
+ return {v1, v2};
+#else
BAIDU_SCOPED_LOCK(const_cast<FastPthreadMutex&>(_mutex));
return _value;
-#endif // __x86_64__ || __ARM_NEON
+#endif
}
void AtomicInteger128::store(Value value) {
-#if __x86_64__
+#ifdef __x86_64__
+ (void)_seq;
__m128i v = _mm_load_si128(reinterpret_cast<__m128i*>(&value));
_mm_store_si128(reinterpret_cast<__m128i*>(&_value), v);
-#elif __ARM_NEON
+#elif defined(__ARM_NEON)
+ (void)_seq;
int64x2_t v = vld1q_s64(reinterpret_cast<int64_t*>(&value));
vst1q_s64(reinterpret_cast<int64_t*>(&_value), v);
+#elif defined(__riscv)
Review Comment:
Same as above: this block uses RV64-only `ld`/`sd`, so it should be guarded
with `__riscv_xlen == 64` to avoid RV32 build failures (and keep the mutex
fallback for RV32).
##########
src/bthread/task_group.cpp:
##########
@@ -85,37 +85,100 @@ BAIDU_VOLATILE_THREAD_LOCAL(void*, tls_unique_user_ptr,
NULL);
const TaskStatistics EMPTY_STAT = { 0, 0, 0 };
AtomicInteger128::Value AtomicInteger128::load() const {
-#if __x86_64__ || __ARM_NEON
- // Supress compiler warning.
- (void)_mutex;
-#endif // __x86_64__ || __ARM_NEON
-
-#if __x86_64__ || __ARM_NEON
#ifdef __x86_64__
+ (void)_mutex;
+ (void)_seq;
__m128i value = _mm_load_si128(reinterpret_cast<const __m128i*>(&_value));
-#else // __ARM_NEON
+ return {value[0], value[1]};
+#elif defined(__ARM_NEON)
+ (void)_mutex;
+ (void)_seq;
int64x2_t value = vld1q_s64(reinterpret_cast<const int64_t*>(&_value));
-#endif // __x86_64__
return {value[0], value[1]};
-#else // __x86_64__ || __ARM_NEON
- // RISC-V and other architectures use mutex fallback
+#elif defined(__riscv)
+ (void)_mutex;
+ // RISC-V: Seqlock-based atomic 128-bit load.
+ int64_t v1, v2;
+ uint64_t seq0, seq1;
+ do {
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ : "=r"(seq0)
+ : "m"(_seq)
+ : "memory"
+ );
+ if (seq0 & 1) continue;
+ __asm__ volatile("fence r, rw\n\t" ::: "memory");
+ __asm__ volatile(
+ "ld %0, %2\n\t"
+ "ld %1, %3\n\t"
+ : "=r"(v1), "=r"(v2)
+ : "m"(_value.v1), "m"(_value.v2)
+ : "memory"
+ );
+ __asm__ volatile("fence r, rw\n\t" ::: "memory");
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ : "=r"(seq1)
+ : "m"(_seq)
+ : "memory"
+ );
+ } while (seq0 != seq1);
+ return {v1, v2};
+#else
BAIDU_SCOPED_LOCK(const_cast<FastPthreadMutex&>(_mutex));
return _value;
-#endif // __x86_64__ || __ARM_NEON
+#endif
}
void AtomicInteger128::store(Value value) {
-#if __x86_64__
+#ifdef __x86_64__
+ (void)_seq;
__m128i v = _mm_load_si128(reinterpret_cast<__m128i*>(&value));
_mm_store_si128(reinterpret_cast<__m128i*>(&_value), v);
-#elif __ARM_NEON
+#elif defined(__ARM_NEON)
+ (void)_seq;
int64x2_t v = vld1q_s64(reinterpret_cast<int64_t*>(&value));
vst1q_s64(reinterpret_cast<int64_t*>(&_value), v);
+#elif defined(__riscv)
+ (void)_mutex;
+ // RISC-V: Seqlock-based atomic 128-bit store.
+ uint64_t old_seq;
+ __asm__ volatile(
+ "ld %0, %1\n\t"
+ : "=r"(old_seq)
+ : "m"(_seq)
+ : "memory"
+ );
+ uint64_t new_seq = old_seq + 1;
+ __asm__ volatile(
+ "fence w, w\n\t"
+ "sd %0, %1\n\t"
+ :
+ : "r"(new_seq), "m"(_seq)
+ : "memory"
+ );
+ __asm__ volatile("fence w, w\n\t" ::: "memory");
+ __asm__ volatile(
+ "sd %0, %2\n\t"
+ "sd %1, %3\n\t"
+ :
+ : "r"(value.v1), "r"(value.v2),
+ "m"(_value.v1), "m"(_value.v2)
+ : "memory"
+ );
+ __asm__ volatile("fence w, w\n\t" ::: "memory");
+ new_seq++;
+ __asm__ volatile(
+ "sd %0, %1\n\t"
+ :
+ : "r"(new_seq), "m"(_seq)
+ : "memory"
+ );
Review Comment:
The RISC-V inline asm blocks write to `_seq`/`_value.*` but declare those
memory operands as inputs ("m"), so the compiler is not informed that the
memory is being modified. This is undefined behavior and can lead to
miscompilation (e.g., reordering/caching around the asm). Use output
constraints ("=m"/"+m") and preferably named operands to make the intent
explicit.
##########
src/bthread/task_group.cpp:
##########
@@ -85,37 +85,100 @@ BAIDU_VOLATILE_THREAD_LOCAL(void*, tls_unique_user_ptr,
NULL);
const TaskStatistics EMPTY_STAT = { 0, 0, 0 };
AtomicInteger128::Value AtomicInteger128::load() const {
-#if __x86_64__ || __ARM_NEON
- // Supress compiler warning.
- (void)_mutex;
-#endif // __x86_64__ || __ARM_NEON
-
-#if __x86_64__ || __ARM_NEON
#ifdef __x86_64__
+ (void)_mutex;
+ (void)_seq;
__m128i value = _mm_load_si128(reinterpret_cast<const __m128i*>(&_value));
-#else // __ARM_NEON
+ return {value[0], value[1]};
+#elif defined(__ARM_NEON)
+ (void)_mutex;
+ (void)_seq;
int64x2_t value = vld1q_s64(reinterpret_cast<const int64_t*>(&_value));
-#endif // __x86_64__
return {value[0], value[1]};
-#else // __x86_64__ || __ARM_NEON
- // RISC-V and other architectures use mutex fallback
+#elif defined(__riscv)
Review Comment:
This RISC-V path uses RV64-only instructions (`ld`/`sd`). Guarding only with
`defined(__riscv)` will break RV32 builds. Please restrict this path to
`__riscv_xlen == 64` (and fall back to the mutex otherwise).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
