Skip to content

feat: support xLLM CUDA backend integration with tilelang kernels.#980

Draft
Wang-1F wants to merge 1 commit intojd-opensource:mainfrom
Wang-1F:test_tilelang
Draft

feat: support xLLM CUDA backend integration with tilelang kernels.#980
Wang-1F wants to merge 1 commit intojd-opensource:mainfrom
Wang-1F:test_tilelang

Conversation

@Wang-1F
Copy link
Collaborator

@Wang-1F Wang-1F commented Mar 3, 2026

deaft

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for TileLang CUDA kernels, which involves substantial modifications to the CMake build configuration and the CUDA Dockerfile to handle new dependencies like TVM and TileLang. Additionally, new integration tests have been added to validate the FlashInfer and TileLang kernel functionality. My review identified two critical issues: one in the Dockerfile where the LD_LIBRARY_PATH is incorrectly overwritten, and another in a new test file that contains a hardcoded, user-specific absolute path, which compromises test portability.

Comment on lines +222 to 223
ENV LD_LIBRARY_PATH=/usr/local/tilelang/lib:$LD_LIBRARY_PATH
ENV LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The LD_LIBRARY_PATH is being overwritten. The ENV instruction on line 223 replaces the value set on line 222, which means the path /usr/local/tilelang/lib will be missing from the final LD_LIBRARY_PATH. This will likely cause runtime errors as libraries from TileLang will not be found. To fix this, you should combine them into a single ENV instruction to correctly prepend both paths.

ENV LD_LIBRARY_PATH=/usr/local/tilelang/lib:/usr/local/lib:$LD_LIBRARY_PATH

Comment on lines +678 to +683
const char* so_env = std::getenv("TILELANG_TMA_KERNEL_SO_PATH");
const std::string so_path =
(so_env != nullptr && std::string(so_env).size() > 0)
? std::string(so_env)
: "/export/home/wangyifan/tmp/tilelang/test_tilelang/kernels/"
"simple_matmul.so";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This test includes a hardcoded absolute path (/export/home/wangyifan/...) as a fallback. This makes the test non-portable, as it will fail on any machine other than the original author's or in any CI environment. The test should instead be skipped if the required TILELANG_TMA_KERNEL_SO_PATH environment variable is not set.

Suggested change
const char* so_env = std::getenv("TILELANG_TMA_KERNEL_SO_PATH");
const std::string so_path =
(so_env != nullptr && std::string(so_env).size() > 0)
? std::string(so_env)
: "/export/home/wangyifan/tmp/tilelang/test_tilelang/kernels/"
"simple_matmul.so";
const char* so_env = std::getenv("TILELANG_TMA_KERNEL_SO_PATH");
if (!so_env || !*so_env) {
GTEST_SKIP() << "TILELANG_TMA_KERNEL_SO_PATH is not set, skipping TMA matmul test.";
}
const std::string so_path(so_env);

std::string backend;
};

class FlashinferRealPlanTest : public ::testing::TestWithParam<PlanTestConfig> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlashinferRealPlanTest和这个提交有什么关系?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants