[Build] Show error message when using ROCm with LTO and different compilers - #35232
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
There was a problem hiding this comment.
Pull request overview
Fixes ROCm/HIP builds with LTO when the default C++ compiler is GCC but HIP is using Clang, preventing missing PyInit_* symbols in generated Python extension shared libraries.
Changes:
- Detect LTO usage in HIP builds and attempt to align the C++ compiler with the HIP compiler.
- Set
CMAKE_CXX_COMPILERtoCMAKE_HIP_COMPILERwhen-fltois present.
Comments suppressed due to low confidence (1)
CMakeLists.txt:234
- The regex string escapes
-as\\-, which is unnecessary in CMake regex here and reduces readability. Consider usingMATCHES \"-flto\"(or checkingCMAKE_INTERPROCEDURAL_OPTIMIZATION/ IPO settings if that’s how LTO is controlled elsewhere) to make the condition clearer and less fragile.
if (CMAKE_CXX_FLAGS MATCHES "\-flto")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request addresses a critical build failure when using Link-Time Optimization (LTO) on ROCm with a mixed GCC and Clang toolchain. The proposed fix correctly identifies that the C++ compiler must match the HIP compiler (which provides the linker) for LTO to work. My review focuses on making this fix more robust from a build system perspective. While the current approach works, modifying global CMake variables like CMAKE_CXX_COMPILER mid-configuration can be risky. I've suggested an alternative that makes the requirement explicit by failing the build with a clear error message if the compilers don't match, which is a safer and more standard practice in CMake.
6cb4e7f to
7a3b272
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Otherwise it will fail at runtime with:
Failed to import from vllm._C with ImportError('dynamic module does not define module export function (PyInit__C)')
Failed to import from vllm._rocm_C with ImportError('dynamic module does not define module export function (PyInit__rocm_C)')
Signed-off-by: Dāvis Mosāns <Dāvis@Atradu.AI>
|
I rebased and updated PR so now it will show error message instead. |
|
@davispuh You need to properly sign-off your commit before we can proceed. CC. @AndreasKaratzas PTAL |
It is properly signed
but looks like your tool doesn't support SMTPUTF8
GitHub's parser can't do it either lmao but that doesn't really matter. |
Purpose
Currently when default compiler
/usr/bin/c++is GCC and when building with LTO (-flto=auto) when ROCm is using Clang then created shared libraries_C.abi3.so_moe_C.abi3.so,_rocm_C.abi3.sowill be missing symbols likePyInit__C.The issue is that
.ofiles contain GCC LTO IR which are ignored when linking with Clang.Test Plan
_C.abi3.so_moe_C.abi3.so,_rocm_C.abi3.soTest Result
Without this PR:
With this PR:
I also tried this:
But unfortunately it doesn't work, so can't set it only for these libraries 😞
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.