gemini-code-assist[bot] commented on code in PR #19593: URL: https://github.com/apache/tvm/pull/19593#discussion_r3282253780
########## CMakeLists.txt: ########## @@ -328,10 +334,10 @@ tvm_file_glob(GLOB RUNTIME_SRCS src/runtime/*.cc src/runtime/vm/*.cc src/runtime/memory/*.cc - src/runtime/disco/*.cc src/runtime/minrpc/*.cc - src/runtime/vm/*.cc ) Review Comment:  The removal of `src/runtime/disco/*.cc` from `RUNTIME_SRCS` causes a regression for Hexagon DSP builds (`BUILD_FOR_HEXAGON`). As noted in `cmake/modules/Hexagon.cmake`, Hexagon builds expect a monolithic runtime where all sources are folded into `libtvm_runtime`. By moving disco to `libtvm_runtime_extra`, it will be missing from the Hexagon runtime. Please ensure disco sources are added back to `RUNTIME_SRCS` when building for Hexagon. ``` src/runtime/*.cc src/runtime/vm/*.cc src/runtime/memory/*.cc src/runtime/minrpc/*.cc ) if(BUILD_FOR_HEXAGON) tvm_file_glob(GLOB _disco_srcs src/runtime/disco/*.cc) list(APPEND RUNTIME_SRCS ${_disco_srcs}) endif() ``` ########## CMakeLists.txt: ########## @@ -471,6 +443,122 @@ include(cmake/modules/contrib/ExampleNPU.cmake) include(cmake/modules/contrib/vllm.cmake) include(cmake/modules/Git.cmake) +# ---- libtvm_runtime_extra assembly ---- Review Comment:  The `libtvm_runtime_extra` assembly block should be gated to avoid creating a shared library when building for the Hexagon DSP (`BUILD_FOR_HEXAGON`). Hexagon DSP builds typically use static linking and a monolithic runtime, and the standard shared library mechanism used here may not be appropriate for the DSP environment. ``` # ---- libtvm_runtime_extra assembly ---- if(NOT BUILD_FOR_HEXAGON) ``` ########## CMakeLists.txt: ########## @@ -471,6 +443,122 @@ include(cmake/modules/contrib/ExampleNPU.cmake) include(cmake/modules/contrib/vllm.cmake) include(cmake/modules/Git.cmake) +# ---- libtvm_runtime_extra assembly ---- +# Must be here (after all contrib cmake files) so RUNTIME_EXTRA_SRCS and +# TVM_RUNTIME_EXTRA_LINKER_LIBS are fully populated. +# Disco core sources. +tvm_file_glob(GLOB _disco_core_srcs src/runtime/disco/*.cc) +list(APPEND RUNTIME_EXTRA_SRCS ${_disco_core_srcs}) + +# Distributed disco (disabled for Hexagon cross-compile). +if(NOT BUILD_FOR_HEXAGON) + tvm_file_glob(GLOB _disco_dist_srcs src/runtime/disco/distributed/*.cc) + list(APPEND RUNTIME_EXTRA_SRCS ${_disco_dist_srcs}) +endif() + +# NCCL / cuda_ipc — requires CUDA + NCCL. +if(USE_CUDA AND USE_NCCL) + find_nccl(${USE_NCCL}) + include_directories(SYSTEM ${NCCL_INCLUDE_DIR}) + tvm_file_glob(GLOB _nccl_srcs src/runtime/disco/nccl/*.cc src/runtime/disco/cuda_ipc/*.cc 3rdparty/tensorrt_llm/*.cu) + set_source_files_properties(src/runtime/disco/nccl/nccl.cc PROPERTIES COMPILE_DEFINITIONS "TVM_NCCL_RCCL_SWITCH=0") + list(APPEND RUNTIME_EXTRA_SRCS ${_nccl_srcs}) +endif() + +# NVSHMEM. +if(USE_CUDA AND USE_NVSHMEM) + find_nvshmem(${USE_NVSHMEM}) + if(NOT NVSHMEM_FOUND) + message(FATAL_ERROR "Cannot find NVSHMEM, USE_NVSHMEM=" ${USE_NVSHMEM}) + endif() + set(CMAKE_CUDA_SEPARABLE_COMPILATION ON) + set(CMAKE_POSITION_INDEPENDENT_CODE ON) + tvm_file_glob(GLOB _nvshmem_srcs src/runtime/contrib/nvshmem/*.cc src/runtime/contrib/nvshmem/*.cu) + list(APPEND RUNTIME_EXTRA_SRCS ${_nvshmem_srcs}) +endif() + +# RCCL. +if(USE_ROCM AND USE_RCCL) + find_rccl(${USE_RCCL}) + include_directories(SYSTEM ${RCCL_INCLUDE_DIR}) + tvm_file_glob(GLOB _rccl_srcs src/runtime/disco/nccl/*.cc) + set_source_files_properties(src/runtime/disco/nccl/nccl.cc PROPERTIES COMPILE_DEFINITIONS "TVM_NCCL_RCCL_SWITCH=1") + list(APPEND RUNTIME_EXTRA_SRCS ${_rccl_srcs}) +endif() + +# Bail out early if nothing to compile. +if(NOT RUNTIME_EXTRA_SRCS AND NOT TVM_RUNTIME_EXTRA_EXT_OBJS) + message(STATUS "libtvm_runtime_extra: no sources, skipping target creation") +else() + message(STATUS "Build runtime extras") + + add_library(tvm_runtime_extra_objs OBJECT ${RUNTIME_EXTRA_SRCS}) Review Comment:  `add_library(... OBJECT ...)` will fail if `RUNTIME_EXTRA_SRCS` is empty. While `src/runtime/disco/*.cc` is currently added to this list, it is safer to guard the creation of the object library and the subsequent shared library to handle cases where no sources are found (e.g., in custom or future configurations where disco might be disabled). The check at line 490 already considers the possibility of empty sources, but the implementation below it assumes `tvm_runtime_extra_objs` always exists. ``` if(RUNTIME_EXTRA_SRCS) add_library(tvm_runtime_extra_objs OBJECT ${RUNTIME_EXTRA_SRCS}) target_link_libraries(tvm_runtime_extra_objs PUBLIC tvm_ffi_header) if(TVM_VISIBILITY_FLAG) target_compile_options(tvm_runtime_extra_objs PRIVATE "${TVM_VISIBILITY_FLAG}") endif() endif() add_library(tvm_runtime_extra SHARED $<$<BOOL:${RUNTIME_EXTRA_SRCS}>:$<TARGET_OBJECTS:tvm_runtime_extra_objs>> ${TVM_RUNTIME_EXTRA_EXT_OBJS} ) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
