[Fix] Fix buffer re-import typo in tilelang.languge #1214
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! 🚀 |
WalkthroughA new test file for issue 1198 is added, and type annotations in builtin.py are updated to consistently use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testing/python/issue/test_tilelang_issue_1198.py (1)
1-10: Test implementation looks good.The test correctly validates that
T.Bufferis accessible from thetilelang.languagemodule. The implementation is straightforward and appropriate for a regression test.Optional style note: Line 6 has a trailing comma in the list
[32,]. While syntactically correct, it's typically used for multi-element lists. Consider[32]for consistency with single-element list conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
testing/python/issue/test_tilelang_issue_1198.py(1 hunks)tilelang/language/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
tilelang/language/__init__.py
85-85: Redefinition of unused Buffer from line 19
Remove definition: Buffer
(F811)
85-85: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quick Lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tilelang/language/builtin.py (2)
601-630: LGTM - Type annotations updated correctlyThe function signature and type checks have been properly updated to use
tir.Buffer. The logic correctly validates the descriptor shape and converts it to a BufferLoad when needed.Consider extracting the long error message to improve code style:
- if not isinstance(descriptor, (BufferLoad, tir.Buffer)): - raise TypeError("Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad.") + DESCRIPTOR_TYPE_ERROR = "Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad." + if not isinstance(descriptor, (BufferLoad, tir.Buffer)): + raise TypeError(DESCRIPTOR_TYPE_ERROR)
632-664: LGTM - Consistent type annotation updatesThe function follows the same pattern as
initialize_wgmma_descriptorwith proper type checks and validation.Consider extracting the long error message for consistency:
- if not isinstance(descriptor, (BufferLoad, tir.Buffer)): - raise TypeError("Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad.") + DESCRIPTOR_TYPE_ERROR = "Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad." + if not isinstance(descriptor, (BufferLoad, tir.Buffer)): + raise TypeError(DESCRIPTOR_TYPE_ERROR)testing/python/issue/test_tilelang_issue_1198.py (1)
5-11: Consider enhancing the test to verify the fixThe test successfully validates that the syntax is correct, but it doesn't actually exercise any of the changed functions (e.g.,
warpgroup_fence_operand,initialize_wgmma_descriptor) that now usetir.Buffer.Consider adding a test that actually uses one of the updated functions to ensure the buffer type handling works correctly:
def test_issue_1198(): @T.prim_func def foo(x: T.Buffer([32], "int32")): # Test that buffer can be used with updated function signatures T.evaluate(T.tir.call_intrin("handle", "dummy", x))Or test one of the descriptor functions if applicable to your testing environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
testing/python/issue/test_tilelang_issue_1198.py(1 hunks)tilelang/language/builtin.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
tilelang/language/builtin.py
611-611: Avoid specifying long messages outside the exception class
(TRY003)
644-644: Avoid specifying long messages outside the exception class
(TRY003)
679-679: Avoid specifying long messages outside the exception class
(TRY003)
681-682: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quick Lint
🔇 Additional comments (2)
tilelang/language/builtin.py (2)
11-11: LGTM - Consistent import patternThe removal of
Bufferfrom the direct import is correct, as the code now consistently usestir.Bufferthroughout the file.
433-490: LGTM - Correct type annotation updateThe function signature and isinstance check at line 459 have been correctly updated to use
tir.Buffer, maintaining consistency with the import changes.
| if not isinstance(descriptor, (BufferLoad, tir.Buffer)): | ||
| raise TypeError("Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad.") | ||
|
|
||
| if isinstance(descriptor, Buffer) and len(descriptor.shape) != 1 or descriptor.shape[0] != 1: | ||
| if isinstance(descriptor, tir.Buffer) and len( | ||
| descriptor.shape) != 1 or descriptor.shape[0] != 1: | ||
| raise ValueError("Descriptor must be a 1D buffer of size 1.") |
There was a problem hiding this comment.
Fix operator precedence issue in condition
Lines 681-682 have a logic error due to operator precedence. The condition:
if isinstance(descriptor, tir.Buffer) and len(descriptor.shape) != 1 or descriptor.shape[0] != 1:Evaluates as: (isinstance(descriptor, tir.Buffer) and len(descriptor.shape) != 1) or (descriptor.shape[0] != 1)
This could cause an AttributeError when descriptor is a BufferLoad, as the second part of the or evaluates without the isinstance guard.
Apply this diff to fix the precedence:
- if isinstance(descriptor, tir.Buffer) and len(
- descriptor.shape) != 1 or descriptor.shape[0] != 1:
+ if isinstance(descriptor, tir.Buffer) and (len(descriptor.shape) != 1 or descriptor.shape[0] != 1):Additionally, consider extracting the error message:
- if not isinstance(descriptor, (BufferLoad, tir.Buffer)):
- raise TypeError("Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad.")
+ DESCRIPTOR_TYPE_ERROR = "Descriptor must be a tvm.tir.Buffer or tvm.tir.BufferLoad."
+ if not isinstance(descriptor, (BufferLoad, tir.Buffer)):
+ raise TypeError(DESCRIPTOR_TYPE_ERROR)🧰 Tools
🪛 Ruff (0.14.3)
679-679: Avoid specifying long messages outside the exception class
(TRY003)
681-682: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
683-683: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In tilelang/language/builtin.py around lines 678 to 683, the conditional has an
operator precedence bug causing the descriptor.shape[0] check to run for
non-Buffer objects; change the condition to guard both shape checks behind the
isinstance test by grouping the shape checks with parentheses (e.g. if
isinstance(descriptor, tir.Buffer) and (len(descriptor.shape) != 1 or
descriptor.shape[0] != 1): raise ValueError(...)), and extract the ValueError
message into a small named variable or constant above the check so the message
is declared once and reused.
* Fix Buffer re-import typo in tilelang.langugage * fix lint error
2 participants
This is a fix of #1198
Summary by CodeRabbit
Tests
Refactor