Skip to content

Conversation

@liuchenbing
Copy link
Contributor

@liuchenbing liuchenbing commented Nov 13, 2025

What this PR does / why we need it?

Does this PR introduce any user-facing change?

How was this patch tested?

…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
…n vllm-ascend.

vLLM version: v0.11.0
vLLM main: vllm-project/vllm
Signed-off-by: liuchenbing <[email protected]>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@liuchenbing liuchenbing changed the title WIP-test test-310p Nov 13, 2025
@liuchenbing liuchenbing changed the title test-310p test-lora-310p Nov 13, 2025
Copy link
Contributor

@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 aims to adjust the conditional compilation for bfloat16_t support in several C++ kernels. However, the changes introduce compilation errors across all modified C++ files (bgmv_expand.cpp, bgmv_shrink.cpp, sgmv_expand.cpp, sgmv_shrink.cpp) due to an invalid preprocessor directive syntax and a typo in a macro name. I've left comments with suggestions to fix these critical issues. The change in vllm_ascend/lora/punica_npu.py appears to be a valid fix to ensure the correct data type for a kernel input.

Comment on lines +345 to +346
#ifndef __CCE_AI_CORE__ || __CCE_AI_CORE__ >= 200
BGMV_EXPAND_TYPE_DECLARE(bfloat16_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This preprocessor directive contains a typo and uses invalid syntax. The macro name should be __CCE_AICORE__ (with a single underscore). Also, #ifndef A || B is not valid; you should use #if for compound conditions. The same issue exists in the other conditional compilation block in this file.

#if !defined(__CCE_AICORE__) || (__CCE_AICORE__ >= 200)
    BGMV_EXPAND_TYPE_DECLARE(bfloat16_t);

Comment on lines +229 to +230
#ifndef __CCE_AI_CORE__ || __CCE_AI_CORE__ >= 200
BGMV_SHRINK_TYPE_DECLARE(bfloat16_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

There's a typo in the macro name (__CCE_AI_CORE__ should be __CCE_AICORE__) and the preprocessor syntax is incorrect. #ifndef does not support ||. Please use #if !defined(...) || ... to fix the compilation error. This correction is needed in both conditional blocks in this file.

#if !defined(__CCE_AICORE__) || (__CCE_AICORE__ >= 200)
    BGMV_SHRINK_TYPE_DECLARE(bfloat16_t);

Comment on lines +360 to +361
#ifndef __CCE_AI_CORE__ || __CCE_AI_CORE__ >= 200
SGMV_EXPAND_TYPE_DECLARE(bfloat16_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This preprocessor directive will cause a compilation failure. There is a typo in the macro name (__CCE_AI_CORE__ should be __CCE_AICORE__), and #ifndef cannot be used with a logical OR. Please rewrite it using #if. This issue is repeated later in the file.

#if !defined(__CCE_AICORE__) || (__CCE_AICORE__ >= 200)
SGMV_EXPAND_TYPE_DECLARE(bfloat16_t);

Comment on lines +245 to +246
#ifndef __CCE_AI_CORE__ || __CCE_AI_CORE__ >= 200
SGMV_SHRINK_TYPE_DECLARE(bfloat16_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

This line has an invalid preprocessor directive and a typo. The macro name should be __CCE_AICORE__. The #ifndef A || B syntax is incorrect and should be replaced with #if !defined(A) || (B). This needs to be fixed in both places it occurs in this file to avoid build errors.

#if !defined(__CCE_AICORE__) || (__CCE_AICORE__ >= 200)
    SGMV_SHRINK_TYPE_DECLARE(bfloat16_t);

@liuchenbing liuchenbing changed the title test-lora-310p test-lora-310p-build Nov 13, 2025
@liuchenbing liuchenbing reopened this Nov 14, 2025
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.

1 participant