[libc-commits] [PATCH] D155511: [libc] Treat the locks array as a bitfield

[Jon Chesterfield via Phabricator via libc-commits]

JonChesterfield added a comment.

Some nits. Still thinking about the tradeoffs between a type and functions but leaning in this direction



================
Comment at: libc/src/__support/RPC/rpc.h:63
 //       progress guarantees on the GPU.
-constexpr uint64_t DEFAULT_PORT_COUNT = 64;
+constexpr uint64_t DEFAULT_PORT_COUNT = 512;
 
----------------
Maybe rename this while we're in the area, it's a maximum port count, not a default. Could static assert that it's a multiple of 32


================
Comment at: libc/src/__support/RPC/rpc.h:165
+  /// The lock is held when the nth bit of the lock bitfield is set, otherwise
+  /// it is availible.
   [[clang::convergent]] LIBC_INLINE bool try_lock(uint64_t lane_mask,
----------------
available


================
Comment at: libc/src/__support/RPC/rpc.h:252
+    const uint32_t slot = index / (sizeof(uint32_t) * 8);
+    const uint32_t bit = index % (sizeof(uint32_t) * 8);
+    return bits[slot].fetch_or(cond << bit, cpp::MemoryOrder::RELAXED) &
----------------
the bitshifted boolean is probably ok - bool << unsigned probably promotes to an unsigned op - but could you either cast the cond or assign it to a uint32 before the expression to remove the ambiguity?


================
Comment at: libc/src/__support/RPC/rpc.h:262
+    const uint32_t bit = index % (sizeof(uint32_t) * 8);
+    return bits[slot].fetch_and(-1u ^ (cond << bit),
+                                cpp::MemoryOrder::RELAXED) &
----------------
UINT32_MAX or ~0u please


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155511/new/

https://reviews.llvm.org/D155511