[BugFix] Scalarize vectorized bool with more than 4 lanes - #2895
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! 🚀 |
📝 WalkthroughWalkthroughThe change fixes boolean reduction identities, scalarizes wide boolean vectors during vectorization, and adds CUDA regression tests for boolean reductions and numeric ChangesBoolean CUDA code generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
src/transform/vectorize_loop.cc (1)
307-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a descriptive name for the transformed expression.
Replace
retwith a name such asvectorized_expr. The name must identify the expression state at the scalarization check.As per path instructions, parameters and local variables should use descriptive lower_snake names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transform/vectorize_loop.cc` around lines 307 - 314, Rename the local variable ret in the ExprFunctor::VisitExpr flow to a descriptive lower_snake_case name such as vectorized_expr, and update its uses in the vectorized boolean scalarization check and return statement.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testing/python/issue/test_tilelang_issue_2568.py`:
- Line 24: Rename the unused kernel bindings in the T.Kernel context manager
from bx and by to _bx and _by, preserving the existing kernel body and test
behavior.
---
Nitpick comments:
In `@src/transform/vectorize_loop.cc`:
- Around line 307-314: Rename the local variable ret in the
ExprFunctor::VisitExpr flow to a descriptive lower_snake_case name such as
vectorized_expr, and update its uses in the vectorized boolean scalarization
check and return statement.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58a28e29-ec6d-4bb7-8985-1cbba9831e26
📒 Files selected for processing (3)
src/backend/common/op/reduce.hsrc/transform/vectorize_loop.cctesting/python/issue/test_tilelang_issue_2568.py
| def _make_reduce_kernel(dtype, reduce_fn): | ||
| @T.prim_func | ||
| def main(A: T.Tensor((M, N), dtype), Out: T.Tensor((M,), dtype)): | ||
| with T.Kernel(1, 1, threads=THREADS) as (bx, by): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the unused kernel bindings.
Ruff reports bx and by as unused. Rename them to _bx and _by to keep this regression test lint-clean.
Proposed fix
- with T.Kernel(1, 1, threads=THREADS) as (bx, by):
+ with T.Kernel(1, 1, threads=THREADS) as (_bx, _by):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with T.Kernel(1, 1, threads=THREADS) as (bx, by): | |
| with T.Kernel(1, 1, threads=THREADS) as (_bx, _by): |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 24-24: Unpacked variable bx is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 24-24: Unpacked variable by is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@testing/python/issue/test_tilelang_issue_2568.py` at line 24, Rename the
unused kernel bindings in the T.Kernel context manager from bx and by to _bx and
_by, preserving the existing kernel body and test behavior.
Source: Linters/SAST tools
|
I think there might be multiple (at least two different) ways to support vectorized bools (e.g.,
For me, the first approach seems most straightforward and simplest to implement. Some notes on the CI failure: since #2874 we have ROCm CI re-enabled, and #2889 fixes an ROCm bug that is missed by #2843 |
|
@jjppp , the failing ROCm check here isn't caused by this PR. test_tilelang_issue_2682.py::test_vectorized_select is currently failing on main for everyone on gfx942 (1 failed, 1830 passed in your run) — it's a HIP codegen gap from |
f4b01a5 to
fbf020b
Compare
Fixes #2568, fixes #2206
Summary
0formaxand logical OR.1formin, bitwise AND, and logical AND.int32andfloat32reductions.C++ style / lint notes
reduce.handvectorize_loop.cc.docs/developer_guide/cpp_style.mdare indicated.