[libc] Move libc server handlers to a shared header #117908
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: We can simply include this header from the shared directory now and do not need to have this level of indirection. Simply stash it with the other libc opcode handlers. If we were able to move the printf handlers to the shared directory then this could just be a header as well, which would HEAVILY simplify the mess associated with building the RPC server first in the projects build, then copying it to the runtimes build.
jhuber6
requested review from
JonChesterfield,
arsenm,
jdoerfert,
lntue,
michaelrj-google and
shiltian
Member
|
@llvm/pr-subscribers-offload @llvm/pr-subscribers-libc Author: Joseph Huber (jhuber6) ChangesSummary: If we were able to move the printf handlers to the shared directory then Full diff: https://github.com/llvm/llvm-project/pull/117908.diff 7 Files Affected:
diff --git a/libc/shared/rpc_opcodes.h b/libc/shared/rpc_opcodes.h
index 430b53aa1870ca..e7ec0e739a7787 100644
--- a/libc/shared/rpc_opcodes.h
+++ b/libc/shared/rpc_opcodes.h
@@ -9,6 +9,8 @@
#ifndef LLVM_LIBC_SHARED_RPC_OPCODES_H
#define LLVM_LIBC_SHARED_RPC_OPCODES_H
+#include "rpc.h"
+
#define LLVM_LIBC_RPC_BASE 'c'
#define LLVM_LIBC_OPCODE(n) (LLVM_LIBC_RPC_BASE << 24 | n)
@@ -46,4 +48,12 @@ typedef enum {
RPC_LAST = 0xFFFFFFFF,
} rpc_opcode_t;
+#undef LLVM_LIBC_OPCODE
+
+namespace rpc {
+// The implementation of this function currently lives in the utility directory
+// at 'utils/gpu/server/rpc_server.cpp'.
+rpc::Status handle_libc_opcodes(rpc::Server::Port &port, uint32_t num_lanes);
+} // namespace rpc
+
#endif // LLVM_LIBC_SHARED_RPC_OPCODES_H
diff --git a/libc/utils/gpu/loader/Loader.h b/libc/utils/gpu/loader/Loader.h
index 497f8114728bf9..6c4434a1b63887 100644
--- a/libc/utils/gpu/loader/Loader.h
+++ b/libc/utils/gpu/loader/Loader.h
@@ -9,8 +9,6 @@
#ifndef LLVM_LIBC_UTILS_GPU_LOADER_LOADER_H
#define LLVM_LIBC_UTILS_GPU_LOADER_LOADER_H
-#include "utils/gpu/server/llvmlibc_rpc_server.h"
-
#include "include/llvm-libc-types/test_rpc_opcodes_t.h"
#include "shared/rpc.h"
@@ -183,7 +181,7 @@ inline uint32_t handle_server(rpc::Server &server, uint32_t index,
break;
}
default:
- status = libc_handle_rpc_port(&*port, num_lanes);
+ status = handle_libc_opcodes(*port, num_lanes);
break;
}
diff --git a/libc/utils/gpu/server/CMakeLists.txt b/libc/utils/gpu/server/CMakeLists.txt
index 36a4c2a8b051ec..b1cada44cd3279 100644
--- a/libc/utils/gpu/server/CMakeLists.txt
+++ b/libc/utils/gpu/server/CMakeLists.txt
@@ -23,9 +23,6 @@ target_compile_definitions(llvmlibc_rpc_server PUBLIC
LIBC_NAMESPACE=${LIBC_NAMESPACE})
# Install the server and associated header.
-install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/llvmlibc_rpc_server.h
- DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
- COMPONENT libc-headers)
install(TARGETS llvmlibc_rpc_server
ARCHIVE DESTINATION "lib${LLVM_LIBDIR_SUFFIX}"
COMPONENT libc)
diff --git a/libc/utils/gpu/server/llvmlibc_rpc_server.h b/libc/utils/gpu/server/llvmlibc_rpc_server.h
deleted file mode 100644
index b7f173734345c0..00000000000000
--- a/libc/utils/gpu/server/llvmlibc_rpc_server.h
+++ /dev/null
@@ -1,24 +0,0 @@
-//===-- Shared memory RPC server instantiation ------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_LIBC_UTILS_GPU_SERVER_RPC_SERVER_H
-#define LLVM_LIBC_UTILS_GPU_SERVER_RPC_SERVER_H
-
-#include <stdint.h>
-
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-int libc_handle_rpc_port(void *port, uint32_t num_lanes);
-
-#ifdef __cplusplus
-}
-#endif
-
-#endif
diff --git a/libc/utils/gpu/server/rpc_server.cpp b/libc/utils/gpu/server/rpc_server.cpp
index 21af7ad7d5f1fd..559e0f06ff0cf1 100644
--- a/libc/utils/gpu/server/rpc_server.cpp
+++ b/libc/utils/gpu/server/rpc_server.cpp
@@ -17,8 +17,6 @@
#include "shared/rpc.h"
#include "shared/rpc_opcodes.h"
-#include "llvmlibc_rpc_server.h"
-
#include "src/__support/arg_list.h"
#include "src/stdio/printf_core/converter.h"
#include "src/stdio/printf_core/parser.h"
@@ -445,15 +443,19 @@ rpc::Status handle_port_impl(rpc::Server::Port &port) {
return rpc::SUCCESS;
}
-int libc_handle_rpc_port(void *port, uint32_t num_lanes) {
+namespace rpc {
+// The implementation of this function currently lives in the utility directory
+// at 'utils/gpu/server/rpc_server.cpp'.
+rpc::Status handle_libc_opcodes(rpc::Server::Port &port, uint32_t num_lanes) {
switch (num_lanes) {
case 1:
- return handle_port_impl<1>(*reinterpret_cast<rpc::Server::Port *>(port));
+ return handle_port_impl<1>(port);
case 32:
- return handle_port_impl<32>(*reinterpret_cast<rpc::Server::Port *>(port));
+ return handle_port_impl<32>(port);
case 64:
- return handle_port_impl<64>(*reinterpret_cast<rpc::Server::Port *>(port));
+ return handle_port_impl<64>(port);
default:
return rpc::ERROR;
}
}
+} // namespace rpc
diff --git a/offload/plugins-nextgen/common/CMakeLists.txt b/offload/plugins-nextgen/common/CMakeLists.txt
index 3ed5c02ed4a3bb..93cf42c89f321e 100644
--- a/offload/plugins-nextgen/common/CMakeLists.txt
+++ b/offload/plugins-nextgen/common/CMakeLists.txt
@@ -32,10 +32,7 @@ elseif(${LIBOMPTARGET_GPU_LIBC_SUPPORT})
target_link_libraries(PluginCommon PRIVATE ${llvmlibc_rpc_server})
target_compile_definitions(PluginCommon PRIVATE LIBOMPTARGET_RPC_SUPPORT)
# We may need to get the headers directly from the 'libc' source directory.
- target_include_directories(PluginCommon PRIVATE
- ${CMAKE_SOURCE_DIR}/../libc/utils/gpu/server
- ${CMAKE_SOURCE_DIR}/../libc/
- ${CMAKE_SOURCE_DIR}/../libc/include)
+ target_include_directories(PluginCommon PRIVATE ${CMAKE_SOURCE_DIR}/../libc/)
endif()
endif()
diff --git a/offload/plugins-nextgen/common/src/RPC.cpp b/offload/plugins-nextgen/common/src/RPC.cpp
index be41928111da44..bcfe32d5eafe7e 100644
--- a/offload/plugins-nextgen/common/src/RPC.cpp
+++ b/offload/plugins-nextgen/common/src/RPC.cpp
@@ -14,7 +14,6 @@
// TODO: This should be included unconditionally and cleaned up.
#if defined(LIBOMPTARGET_RPC_SUPPORT)
-#include "llvmlibc_rpc_server.h"
#include "shared/rpc.h"
#include "shared/rpc_opcodes.h"
#endif
@@ -79,21 +78,21 @@ Error RPCServerTy::runServer(plugin::GenericDeviceTy &Device) {
std::min(Device.requestedRPCPortCount(), rpc::MAX_PORT_COUNT);
rpc::Server Server(NumPorts, Buffers[Device.getDeviceId()]);
- auto port = Server.try_open(Device.getWarpSize());
- if (!port)
+ auto Port = Server.try_open(Device.getWarpSize());
+ if (!Port)
return Error::success();
int Status = rpc::SUCCESS;
- switch (port->get_opcode()) {
+ switch (Port->get_opcode()) {
case RPC_MALLOC: {
- port->recv_and_send([&](rpc::Buffer *Buffer, uint32_t) {
+ Port->recv_and_send([&](rpc::Buffer *Buffer, uint32_t) {
Buffer->data[0] = reinterpret_cast<uintptr_t>(Device.allocate(
Buffer->data[0], nullptr, TARGET_ALLOC_DEVICE_NON_BLOCKING));
});
break;
}
case RPC_FREE: {
- port->recv([&](rpc::Buffer *Buffer, uint32_t) {
+ Port->recv([&](rpc::Buffer *Buffer, uint32_t) {
Device.free(reinterpret_cast<void *>(Buffer->data[0]),
TARGET_ALLOC_DEVICE_NON_BLOCKING);
});
@@ -101,10 +100,10 @@ Error RPCServerTy::runServer(plugin::GenericDeviceTy &Device) {
}
default:
// Let the `libc` library handle any other unhandled opcodes.
- Status = libc_handle_rpc_port(&*port, Device.getWarpSize());
+ Status = handle_libc_opcodes(*Port, Device.getWarpSize());
break;
}
- port->close();
+ Port->close();
if (Status != rpc::SUCCESS)
return createStringError("RPC server given invalid opcode!");
|
shiltian
reviewed
Nov 27, 2024
| switch (num_lanes) { | ||
| case 1: | ||
| return handle_port_impl<1>(*reinterpret_cast<rpc::Server::Port *>(port)); | ||
| return handle_port_impl<1>(port); |
Contributor
There was a problem hiding this comment.
I find it confusing looking at the variable name of libc project. what is the rule here?
Contributor
Author
There was a problem hiding this comment.
This is code that libc exports to handle the opcodes offload doesn't know how to handle.
shiltian
reviewed
Nov 27, 2024
| break; | ||
| } | ||
| port->close(); | ||
| Port->close(); |
Contributor
There was a problem hiding this comment.
and then this becomes Port?
NVM. This is offload.
shiltian
reviewed
Nov 27, 2024
| ${CMAKE_SOURCE_DIR}/../libc/utils/gpu/server | ||
| ${CMAKE_SOURCE_DIR}/../libc/ | ||
| ${CMAKE_SOURCE_DIR}/../libc/include) | ||
| target_include_directories(PluginCommon PRIVATE ${CMAKE_SOURCE_DIR}/../libc/) |
Contributor
There was a problem hiding this comment.
This change is fine, but I wonder why don't we just export libc's rpc as a target and then just link against the target, such that CMake will handle all the interface directories.
Contributor
Author
There was a problem hiding this comment.
You just described the next patch I'm going to do once this lands. We already have something called include(FindLibcCommonUtils) that I will use after this removes the need for the extra directories.
shiltian
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
We can simply include this header from the shared directory now and do
not need to have this level of indirection. Simply stash it with the
other libc opcode handlers.
If we were able to move the printf handlers to the shared directory then
this could just be a header as well, which would HEAVILY simplify the
mess associated with building the RPC server first in the projects
build, then copying it to the runtimes build.