[libc-commits] [PATCH] D145913: [libc] Add initial support for an RPC mechanism for the GPU

Thu Mar 16 08:48:04 PDT 2023

jhuber6 marked an inline comment as done.
jhuber6 added inline comments.

================
Comment at: libc/src/__support/RPC/rpc.h:1
+//===-- Shared memory RPC client / server interface -------------*- C++ -*-===//
+//
----------------
sivachandra wrote:
> Should this RPC directory be nested under `OSUtil/gpu` as it is a GPU specific? AFAICT there is nothing GPU specific though.
Right now it's "gpu specific" but theoretically it could be used by any heterogeneous system that can share a memory region atomically. Maybe we could port it to run on FPGAs some day. But no other targets will use this right now, so it would be fine to put it under a GPU only directory if you want.

================
Comment at: libc/src/__support/RPC/rpc_client.cpp:22
+/// whomever needs to run the server.
+extern "C" [[gnu::visibility("protected")]] const bool __llvm_libc_rpc = 0;
+
----------------
sivachandra wrote:
> `false` ? Also, is this being used anywhere?
This isn't used right now. The idea is that when we provide this as a static library, any entrypoint that requires the RPC will pull in this file to resolve the symbol. That will in turn pull in this externally visible symbol which we can then read from the image directly. Therefore, if the GPU image does not contain `__llvm_libc_rpc` we don't need to bother with the runtime cost of spinning up a server on the host CPU.

It's set to `false` because constants don't get emitted as symbols if they are undefined, and we want to use the default initializer. It might be fine to make it true, but the presence of the symbol is the boolean here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145913