[libc-commits] [PATCH] D148288: [libc] Update RPC interface for system utilities on the GPU

[Johannes Doerfert via Phabricator via libc-commits]

jdoerfert added a comment.

some comments, waiting on the next version now.



================
Comment at: libc/src/__support/RPC/rpc.h:42
+  // TODO: This should be put in a separate buffer to avoid excess padding.
+  uint16_t opcode;
 };
----------------
I would argue data should be int8_t[62] to get the size right.


================
Comment at: libc/src/__support/RPC/rpc.h:153-155
+  // We wait until we own the buffer before sending. We own the buffer if the
+  // input Ack bit is equal to the output Data bit.
+  while (bool(in & Process::Ack) != bool(out & Process::Data)) {
----------------
Complex logic, please put it into a helper with comment. Probably the entire while loop.


================
Comment at: libc/src/__support/RPC/rpc.h:163-164
+  atomic_thread_fence(cpp::MemoryOrder::RELEASE);
+  process.outbox[index].store(out ^ Process::Data, cpp::MemoryOrder::RELAXED);
+  out = out ^ Process::Data;
+}
----------------
reorder to simplify?


================
Comment at: libc/src/__support/RPC/rpc.h:176
+    in = process.inbox[index].load(cpp::MemoryOrder::RELAXED);
   }
+  atomic_thread_fence(cpp::MemoryOrder::ACQUIRE);
----------------
Same as above.


================
Comment at: libc/src/__support/RPC/rpc.h:227
+          size - idx > sizeof(Buffer::data) ? sizeof(Buffer::data) : size - idx;
+      inline_memcpy(reinterpret_cast<uint8_t *>(dst) + idx, buffer->data, len);
+    });
----------------
Here and above, move the cast out the loop, or at least stmt, to make it simpler (to read).


================
Comment at: libc/src/__support/RPC/rpc.h:249
+    return cpp::nullopt;
   }
+
----------------
This should use the helper as well, and pass a retry flag as false, hence give up rather than loop. The scheme above otherwise exists 4 times.


================
Comment at: libc/src/__support/RPC/rpc.h:73
 
+  enum Flags {
+    Data = 0b01, // bit 0.
----------------
lntue wrote:
> Since this class is internally used, shall we make it `enum class` to be a bit safer?  If you don't want to keep adding `Flags::Data`, you can add `using Flags::Data` and `using Flags::Ack`, or simply add `using enum Flags` with C++20 inside `struct Process`.
Enum class or just constexpr constants.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148288