[Enhancement]Support mixed-sign ramp indices in LegalizeNegativeIndex#2222
[Enhancement]Support mixed-sign ramp indices in LegalizeNegativeIndex#2222TerminusAkivili wants to merge 1 commit into
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! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR extends the LegalizeNegativeIndex pass to handle mixed-sign vector ramp indices and block-level buffer region metadata. New helpers detect and rewrite ramp lanes that are provably negative through shuffle reconstruction, while a block visitor applies region-level legalization to buffer metadata. ChangesMixed-Sign Ramp Index Legalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
src/transform/legalize_negative_index.cc (1)
173-173: 💤 Low valueConsider adding a null check for non-constant
lanes.
as_const_intreturnsnullptriframp->lanesis not a constant integer. While the existing analyzer code (line 68) uses the same pattern, dereferencing without a check is technically UB if a symbolic lanes value ever appears.Suggested defensive check
- int lanes = *as_const_int(ramp->lanes); + const int64_t* lanes_ptr = as_const_int(ramp->lanes); + if (lanes_ptr == nullptr) + return PrimExpr(); + int lanes = static_cast<int>(*lanes_ptr);🤖 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/legalize_negative_index.cc` at line 173, The code dereferences the result of as_const_int(ramp->lanes) without checking for nullptr (int lanes = *as_const_int(ramp->lanes)); add a defensive null check around the as_const_int call (referencing ramp->lanes and as_const_int) and handle the non-constant case by skipping the legalization path or returning/continuing appropriately (e.g., treat it as non-constant and leave the node unchanged or bail out from the current transformation); ensure you avoid UB by only dereferencing when the pointer is non-null and preserve existing analyzer behavior used elsewhere in the file.
🤖 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.
Nitpick comments:
In `@src/transform/legalize_negative_index.cc`:
- Line 173: The code dereferences the result of as_const_int(ramp->lanes)
without checking for nullptr (int lanes = *as_const_int(ramp->lanes)); add a
defensive null check around the as_const_int call (referencing ramp->lanes and
as_const_int) and handle the non-constant case by skipping the legalization path
or returning/continuing appropriately (e.g., treat it as non-constant and leave
the node unchanged or bail out from the current transformation); ensure you
avoid UB by only dereferencing when the pointer is non-null and preserve
existing analyzer behavior used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8110274-8ddc-4c65-a03c-0f541296c2fa
📒 Files selected for processing (2)
src/transform/legalize_negative_index.cctesting/python/transform/test_tilelang_transform_legalize_negative_index.py
83eaef1 to
50f210b
Compare
Summary
LegalizeNegativeIndexto handle constantRampindices whose lanes mix negative and non-negative values.Motivation
TileLang already legalizes Python-style negative indices in scalar and all-negative vector cases, for example:
This PR extends the same behavior to the mixed-sign constant ramp case:
The existing pass classifies the whole ramp as unknown-sign, because the vector expression is neither fully negative nor fully non-negative. As a result, the negative lanes are left unlegalized even though each lane is constant and can be handled independently.
Kernel Reproducer
Kernels often split a wrapped circular-buffer window into two contiguous accesses, which is usually the faster lowering shape. This example is not meant to prescribe a preferred kernel style; it is a compact reproducer for the existing negative-index semantics when a vectorized load crosses a circular-buffer boundary.
Complete reproducer:
Before this patch, the transform leaves the mixed-sign access as an unlegalized negative range/ramp:
After this patch, the same transform produces the lane-wise legalized indices:
This keeps the transform behavior consistent across scalar negative indices, all-negative vector ramps, and constant mixed-sign vector ramps.
Implementation
When the sign state is unknown, the rewriter now tries a narrow lane-wise rewrite for constant
Rampindices:buffer_extent + lane.The rewritten vector is represented as
T.Shuffle, which preserves the per-lane indexing semantics without changing truly dynamic unknown-sign cases.Summary by CodeRabbit
New Features
Bug Fixes
Tests