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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/OSUtil/gpu/quick_exit.cpp:21
+  rpc::Port port = rpc::client.open(rpc::EXIT);
+  port.send([&](rpc::Buffer *buffer) { buffer->data[0] = status; });
 
----------------
capture-by-value is better here.


================
Comment at: libc/src/__support/RPC/rpc.h:32
+/// A list of opcodes that we use to invoke certain actions on the server.
+enum Opcode : uint16_t {
   NOOP = 0,
----------------
Can this be `enum class` ?


================
Comment at: libc/src/__support/RPC/rpc.h:73
 
+  enum Flags {
+    Data = 0b01, // bit 0.
----------------
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`.


================
Comment at: libc/src/__support/RPC/rpc.h:151
+  // input Ack bit is equal to the output Data bit.
+  while (bool(in & Process::Ack) ^ bool(out & Process::Data)) {
+    sleep_briefly();
----------------
`a != b` is a safer option for boolean than bitwise-xor.


================
Comment at: libc/src/__support/RPC/rpc.h:200
+  // extra send operation.
+  send([&](Buffer *buffer) { buffer->data[0] = size; });
+  for (uint64_t idx = 0; idx < size; idx += sizeof(Buffer::data)) {
----------------
capture-by-value is better here.


================
Comment at: libc/src/__support/RPC/rpc.h:216
+  uint64_t size = 0;
+  recv([&](Buffer *buffer) { size = buffer->data[0]; });
+  void *dst = alloc(size);
----------------
capture-by-reference is not needed.


================
Comment at: libc/src/__support/RPC/rpc.h:219
+  for (uint64_t idx = 0; idx < size; idx += sizeof(Buffer::data)) {
+    recv([&](Buffer *buffer) {
+      uint64_t len =
----------------
I think capture-by-value is good enough here.


================
Comment at: libc/src/__support/RPC/rpc.h:236
+  // If this port cannot send we cannot open it.
+  if (bool(in & Process::Ack) ^ bool(out & Process::Data))
+    return cpp::nullopt;
----------------
`a != b` is a safer option for boolean than bitwise-xor.


================
Comment at: libc/src/__support/RPC/rpc.h:248
+  // thread hasn't picked it up in the meantime.
+  if (bool(in & Process::Ack) ^ bool(out & Process::Data)) {
+    lock->store(0, cpp::MemoryOrder::RELAXED);
----------------
`a != b` is a safer option for boolean than bitwise-xor.


================
Comment at: libc/src/__support/RPC/rpc.h:258
+LIBC_INLINE Port Client::open(uint16_t opcode) {
+  for (;;) {
+    if (cpp::optional<Port> p = try_open(opcode))
----------------
nit: I actually prefer while (true) to for (;;)


================
Comment at: libc/src/__support/RPC/rpc.h:280
+
+  if (!(bool(in & Data) ^ bool(out & Process::Ack))) {
+    lock->store(0, cpp::MemoryOrder::RELAXED);
----------------
`a != b` is actually a safer `xor` operator for boolean than bitwise-xor `^`.  So logically, this is equivalent to
```
bool(in & Data) == bool(out & Process::Ack)
```
Can you double check if it is correct?


================
Comment at: libc/src/__support/RPC/rpc.h:289
+LIBC_INLINE Port Server::open() {
+  for (;;) {
+    if (cpp::optional<Port> p = try_open())
----------------
nit: I actually prefer `while (true)` to `for (;;)`


================
Comment at: libc/utils/gpu/loader/Server.h:40
+  case __llvm_libc::rpc::Opcode::EXIT: {
+    port->recv([&](__llvm_libc::rpc::Buffer *buffer) {
+      exit(static_cast<int>(buffer->data[0]));
----------------
nothing is captured with this lambda.


================
Comment at: libc/utils/gpu/loader/Server.h:46
+  default:
+    port->recv([&](__llvm_libc::rpc::Buffer *) {
+      /* no-op */
----------------
nothing is captured with this lambda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148288