[libc-commits] [PATCH] D150337: [libc][rpc] Allocate a single block of shared memory instead of three

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added inline comments.


================
Comment at: libc/src/__support/RPC/rpc.h:122
+    this->inbox = reinterpret_cast<cpp::Atomic<uint32_t> *>(
+        (char *)state + (InvertInbox ? s : p));
+    this->outbox = reinterpret_cast<cpp::Atomic<uint32_t> *>(
----------------
Static cast


================
Comment at: libc/src/__support/RPC/rpc.h:122
+    this->inbox = reinterpret_cast<cpp::Atomic<uint32_t> *>(
+        (char *)state + (InvertInbox ? s : p));
+    this->outbox = reinterpret_cast<cpp::Atomic<uint32_t> *>(
----------------
jhuber6 wrote:
> Static cast
This is unrelated, but I think we might want to call `InvertInbox` something more descriptive in the fact that it's the server / client.


================
Comment at: libc/src/__support/RPC/rpc.h:130
+  /// Allocate a single block of memory for use by client and server
+  /// template<size_t N> // N is generally a runtime value
+  /// struct equivalent {
----------------



================
Comment at: libc/src/__support/RPC/rpc.h:273
+  memory_allocated_buffer(uint64_t port_count, uint32_t lane_size) {
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
+    (void)lane_size;
----------------
Not sure we need to bother with the macro, I'm sure the optimizer can handle forwarding 32 or `__AMDGCN_WAVEFRONTSIZE`.


================
Comment at: libc/src/__support/RPC/rpc_util.h:54
+template <typename V, typename A>
+LIBC_INLINE constexpr V align_up(V val, A align) {
   return ((val + V(align) - 1) / V(align)) * V(align);
----------------
Did you format this with a different style?


================
Comment at: libc/startup/gpu/amdgpu/start.cpp:41
 extern "C" [[gnu::visibility("protected"), clang::amdgpu_kernel]] void
-_begin(int argc, char **argv, char **env, void *in, void *out, void *buffer) {
+_begin(int argc, char **argv, char **env, void *shared_state) {
   // We need to set up the RPC client first in case any of the constructors
----------------
I'd like something more like `rpc_shared_buffer` or something so we know what it's for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150337