[None][feat] retune causalConv1d fwd dispatch for varlen and short sequences#12739
Conversation
|
/bot run |
📝 WalkthroughWalkthroughA CUDA kernel optimization that dispatches causal convolution forward operations between two thread configurations (64 vs 128 threads) based on input characteristics such as variable-length sequences and sequence length thresholds. Copyright year updated to 2026. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/kernels/causalConv1d/causalConv1d.cu (1)
356-359: Rename new constants to match repository constant naming convention.Please rename the new
constexprconstants to uppercase snakecase withkprefix for consistency with project rules.Suggested diff
- constexpr int kNarrowThreads = 64; - constexpr int kWideThreads = 128; - constexpr int kNElts = sizeof(input_t) == 4 ? 4 : 8; - constexpr int kShortSeqThreshold = kNarrowThreads * kNElts; + constexpr int kNARROW_THREADS = 64; + constexpr int kWIDE_THREADS = 128; + constexpr int kNELTS = sizeof(input_t) == 4 ? 4 : 8; + constexpr int kSHORT_SEQ_THRESHOLD = kNARROW_THREADS * kNELTS; @@ - bool const preferNarrowKernel = isVarlen || params.seqlen <= kShortSeqThreshold; + bool const preferNarrowKernel = isVarlen || params.seqlen <= kSHORT_SEQ_THRESHOLD; @@ - causal_conv1d_fwd_launch<kNarrowThreads, kWidth, input_t, weight_t>(params, stream); + causal_conv1d_fwd_launch<kNARROW_THREADS, kWidth, input_t, weight_t>(params, stream); @@ - causal_conv1d_fwd_launch<kWideThreads, kWidth, input_t, weight_t>(params, stream); + causal_conv1d_fwd_launch<kWIDE_THREADS, kWidth, input_t, weight_t>(params, stream);As per coding guidelines, "C++ constants should use uppercase snakecase with prefix 'k':
int const kDIGIT_NUM = 10;."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tensorrt_llm/kernels/causalConv1d/causalConv1d.cu` around lines 356 - 359, Rename the four new constexpr names to the project's uppercase-snakecase-with-k-prefix convention: change kNarrowThreads -> kNARROW_THREADS, kWideThreads -> kWIDE_THREADS, kNElts -> kN_ELTS (or kN_ELTS if you prefer to preserve the "N" token), and kShortSeqThreshold -> kSHORT_SEQ_THRESHOLD; update every usage/reference in the file (e.g., in kernels or device code) to the new names so compilation succeeds and coding conventions are followed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/tensorrt_llm/kernels/causalConv1d/causalConv1d.cu`:
- Around line 356-359: Rename the four new constexpr names to the project's
uppercase-snakecase-with-k-prefix convention: change kNarrowThreads ->
kNARROW_THREADS, kWideThreads -> kWIDE_THREADS, kNElts -> kN_ELTS (or kN_ELTS if
you prefer to preserve the "N" token), and kShortSeqThreshold ->
kSHORT_SEQ_THRESHOLD; update every usage/reference in the file (e.g., in kernels
or device code) to the new names so compilation succeeds and coding conventions
are followed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6384b4b-60ce-41a0-9307-b6679dfa7686
📒 Files selected for processing (1)
cpp/tensorrt_llm/kernels/causalConv1d/causalConv1d.cu
|
PR_Github #41689 [ run ] triggered by Bot. Commit: |
nv-guomingz
changed the title
|
PR_Github #41689 [ run ] completed with state
|
|
/bot run |
|
PR_Github #41776 [ run ] triggered by Bot. Commit: |
|
PR_Github #41776 [ run ] completed with state
|
|
/bot run |
|
PR_Github #41827 [ run ] triggered by Bot. Commit: |
|
PR_Github #41827 [ run ] completed with state |
|
/bot run |
|
PR_Github #41865 [ run ] triggered by Bot. Commit: |
|
PR_Github #41865 [ run ] completed with state
|
|
/bot run |
|
PR_Github #41903 [ run ] triggered by Bot. Commit: |
|
PR_Github #41903 [ run ] completed with state
|
Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
nv-guomingz
force-pushed
the
user/guomingz/causalconv1d-varlen-tuning
branch
from
171f3aa to
b6e6ae0
Compare
|
/bot run |
|
PR_Github #41913 [ run ] triggered by Bot. Commit: |
|
PR_Github #41913 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #41932 [ run ] triggered by Bot. Commit: |
|
PR_Github #41932 [ run ] completed with state |
…quences (NVIDIA#12739) Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
…quences (NVIDIA#12739) Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
…quences (NVIDIA#12739) Signed-off-by: nv-guomingz <137257613+nv-guomingz@users.noreply.github.com>
4 participants
Summary by CodeRabbit
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.