[Refactor][Build] Separate CMakeLists into different backends#2114
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
📝 WalkthroughWalkthroughRefactors CMake build configuration by moving backend-specific logic (CUDA, ROCm, Metal) from the top-level CMakeLists.txt into dedicated backend subdirectories. Replaces hardcoded CUDA/HIP conditionals with generalized backend-supplied variables for stub linking, RPATH configuration, and patchelf operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 227-231: The backend include block currently lets per-backend
CMakeLists set scalar variables like TILELANG_ACTIVE_BACKEND_* which overwrite
each other; change the contract to accumulate state: in each backend CMakeLists
(cuda/rocm/metal) replace set(...) of the TILELANG_ACTIVE_BACKEND_* variables
with list(APPEND TILELANG_ACTIVE_BACKEND_<X> ...) so each backend appends its
values, then in the top-level include site iterate the collected lists (for
example using foreach or list(GET)/string(JOIN) as needed) to
consume/emit/link/install all entries instead of reading a single scalar; apply
the same change pattern for the other two include blocks mentioned (the blocks
around the 336-345 and 360-383 regions) so RPATH/patchelf and stub lists are
aggregated rather than overwritten.
In `@src/backend/cuda/CMakeLists.txt`:
- Around line 124-127: The RPATH variable TILELANG_ACTIVE_BACKEND_RPATH_EXTRA is
being set under if(UNIX) (which includes macOS) but is only used for non-Apple
UNIX in the top-level append, so change the guard around setting
TILELANG_ACTIVE_BACKEND_RPATH_EXTRA to only run on Linux by making it if(UNIX
AND NOT APPLE); update the condition that currently wraps the set(...) that
references CUDAToolkit_VERSION_MAJOR so the variable is only created on
non-Apple UNIX systems.
In `@src/backend/metal/CMakeLists.txt`:
- Around line 6-16: The CMake logic incorrectly disables codegen by calling
set(USE_METAL OFF) on non-Apple platforms; remove the set(USE_METAL OFF) line
and instead prevent only the Metal runtime wiring from being built on non-Apple
(e.g., skip adding rt_mod_metal.cc into TILE_LANG_METAL_SRCS or gate its
inclusion behind an APPLE check). Update the block around USE_METAL and the
list(APPEND TILE_LANG_SRCS ${TILE_LANG_METAL_SRCS}) so codegen
(tvm_callback_metal_compile) remains enabled while the platform-specific runtime
source src/target/rt_mod_metal.cc is excluded on non-Apple.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8731fa8-cb68-45fe-8fb0-af611ce20e80
📒 Files selected for processing (4)
CMakeLists.txtsrc/backend/cuda/CMakeLists.txtsrc/backend/metal/CMakeLists.txtsrc/backend/rocm/CMakeLists.txt
Encounter build errorOn Ubuntu 24, Linux 6.17.0-1017-oem with ROCm 7.2.2 (AMD Ryzen™ AI 9 HX 370, GPU 890M, no Nvidia GPU)
CMake Error in CMakeLists.txt:
IMPORTED_LOCATION not set for imported target "CUDA::cuda_driver"
configuration "Release".
CMake Error in CMakeLists.txt:
IMPORTED_LOCATION not set for imported target "CUDA::cuda_driver"
configuration "Release".
CMake Error in CMakeLists.txt:
IMPORTED_LOCATION not set for imported target "CUDA::nvml" configuration
"Release".
CMake Error in CMakeLists.txt:
IMPORTED_LOCATION not set for imported target "CUDA::nvml" configuration
"Release".
|
|
Hi @petersktang, I haven't tested the case where all backend options are enabled. Why might this situation occur? Under normal reasoning, we would generally expect only one backend to be active at a time. |
I am using this capability to cross generate/compile code for both cuda and rocm with the same build. This was done using a local modified copy of TileLang. Now, with the separation of CMakeLists per backend, this feature will be even more interesting. A low end computer can become part of the workflow to generate code and deploy to run on a number of remote high end computers and GPUs. BTW, TileLang code can also be developed & tested first on the low end machine with whatever cpu/gpu available before pushing to run on the high end CPU/GPU cluster, and there can be many separate clusters located globally, on nationwide edge servers and on a fleet of AI enabled drones, each with different hardware builds/generations. |
The fix in CMakeLists.txt for USE_CUDA=ON, USE_ROCM=ON, USE_LLVM=ON
target_include_directories(tilelang_objs PRIVATE ${TILE_LANG_INCLUDES})
target_compile_definitions(tilelang_objs PRIVATE TVM_LOG_CUSTOMIZE=1)
if(TILELANG_RELEASE_BUILD)
target_compile_definitions(tilelang_objs PRIVATE TILELANG_RELEASE_BUILD=1)
endif()
after line# 282 add the below:
if(USE_CUDA)
if(TILELANG_USE_CUDA_STUBS)
target_link_libraries(tvm PRIVATE cuda_stub cudart_stub nvrtc_stub)
else()
# find_package(CUDAToolkit REQUIRED)
unset(TILELANG_ACTIVE_BACKEND_PATCHELF_REMOVE)
target_link_libraries(tvm PRIVATE cuda)
endif()
endif()
|
|
Hi @petersktang, it seems that this problem is fixed in latest main branch. Could you please help me verify this? |
seems resolved. Though I have doubt on should be and should be |
Summary by CodeRabbit