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

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added inline comments.


================
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,
----------------
lntue wrote:
> Can this be `enum class` ?
We could, but since this is passed over the wire I think it's okay to treat it as just an integer.


================
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();
----------------
lntue wrote:
> `a != b` is a safer option for boolean than bitwise-xor.
These are bits, we can't do things like `&` if it's an enum class without a ton of conversions. It should be `uint32_t` however.


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


================
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))
----------------
lntue wrote:
> nit: I actually prefer while (true) to for (;;)
I saw examples of both in the codebase, Maybe we should make a style rule for that.


================
Comment at: libc/src/__support/RPC/rpc.h:280
+
+  if (!(bool(in & Data) ^ bool(out & Process::Ack))) {
+    lock->store(0, cpp::MemoryOrder::RELAXED);
----------------
lntue wrote:
> `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?
The compiler will reduce it to an XOR AFAIK, but I explicitly use the language "bits are not equal" in the logic, so it's a lot clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148288