forked from tile-ai/tilelang
-
Notifications
You must be signed in to change notification settings - Fork 8
[Feat] Support stride in T.serial
#41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Inclusive stepped
T.serialis off‑by‑one when(end - start)is divisible bystep(and whenstart == end)The lowering in
tilelang_visit_formostly matches the documented inclusive semantics, but the extent calculation is short by one iteration whenever(end - start)is an exact multiple ofstep, including thestart == endcase.T.serial(0, 8, 2)is documented to producei = 0, 2, 4, 6, 8, but:end - start = 8extent = T.ceildiv(8, 2) = 4truns0..3, soi = 0, 2, 4, 6and thei = 8iteration is lost.T.serial(5, 5, 1):end - start = 0extent = 0→ no iterations, instead of a singlei = 5iteration.To implement the stated semantics
i = start, start+step, …, <= end, the iteration count should befloor((end - start) / step) + 1, which can be expressed asceildiv((end - start) + 1, step)for positivestep.Consider updating the extent computation and the accompanying comment as follows:
You may also want to:
ceildiv((end - start) + 1, step)instead offloor_div(|end-start|, step)), to avoid confusion.stepmust be a positive constant or IntImm (Line 179–187), even though the type annotation allows a generalPrimExpr. Either narrowing the type inSerialStepSpec/serial()or explicitly documenting this constraint would make behavior clearer.I'll verify the review comment by investigating the codebase. Let me search for the relevant code and test cases.
Length of output: 2618
🏁 Script executed:
Length of output: 778
🏁 Script executed:
Length of output: 198
🏁 Script executed:
Length of output: 3780
🏁 Script executed:
Length of output: 2842
Let me search more thoroughly for test cases and documentation about the expected semantics.
Length of output: 1539
🏁 Script executed:
Length of output: 28
🏁 Script executed:
Length of output: 3508
🏁 Script executed:
Length of output: 2305
🏁 Script executed:
Length of output: 1157
🏁 Script executed:
Length of output: 100
Let me check the testing directories for any tests of stepped serial loops:
Length of output: 64
🏁 Script executed:
Length of output: 2231
🏁 Script executed:
Length of output: 48
Fix off-by-one error in inclusive stepped
T.serialiteration countThe review comment correctly identifies a critical off-by-one bug. The documented semantics specify inclusive iteration (
i = start, start+step, ..., <= endper tilelang/language/tir/ir.py:42-45), but the extent calculation is short by one when(end - start)is divisible bystep(includingstart == end).Verification confirms the mathematical error:
T.serial(0, 8, 2)should producei = 0,2,4,6,8(5 iterations), but current code produces 4 iterations.T.serial(5, 5, 1)should producei = 5(1 iteration), but current code produces 0.ceildiv((end - start) + 1, step_val)correctly implementsfloor((end-start)/step) + 1.Additionally, update the docstring at lines 153–155, which currently states
t in [0, floor_div(|end-start|, step)]but should reflect the correct formula.🧰 Tools
🪛 Ruff (0.14.5)
149-149: Possible hardcoded password assigned to argument: "token"
(S106)
🤖 Prompt for AI Agents