-
Notifications
You must be signed in to change notification settings - Fork 380
[Fix] Remove unused let_bindings_ in CodeGenC to fix #1300 #1305
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
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! 🚀 |
WalkthroughThese changes introduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tilelang/language/v2/builder.py (1)
477-492: LGTM! Ref support added with appropriate deprecation warning.The macro_arg function now accepts both Var and Ref annotations, with a deprecation warning for Var guiding users to the new
T.Refpattern. The validation logic correctly ensures the argument is a local.var-allocated buffer.Consider moving the Ref import to the module level (line 1-23) rather than importing locally within the function:
from tvm.tir.expr import EqualOp, FloatImm, IntImm, NotEqualOp, PrimExpr, StringImm, Var +from tilelang.language.proxy import RefThen remove line 478. This is a minor style preference—local imports are acceptable, but module-level imports improve discoverability.
testing/python/language/test_tilelang_language_frontend_v2.py (1)
397-428: LGTM! Comprehensive T.Ref test coverage.The new test blocks validate T.Ref macro annotations with both successful (alloc_var) and error (plain int) cases, providing parity with the existing T.Var tests. The tests confirm proper script generation and error handling.
The exception message at line 425 could be extracted to a constant to address the static analysis hint (TRY003), though this is consistent with the pattern used at line 393:
ERROR_MSG = "Expect to report an error, x should not be passed as T.Var"However, this is a minor style preference and not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
3rdparty/tvm(1 hunks)testing/python/language/test_tilelang_intimm.py(1 hunks)testing/python/language/test_tilelang_language_frontend_v2.py(1 hunks)tilelang/language/__init__.py(1 hunks)tilelang/language/proxy.py(2 hunks)tilelang/language/v2/builder.py(2 hunks)tilelang/language/v2/dtypes.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
testing/python/language/test_tilelang_language_frontend_v2.py (2)
tilelang/language/v2/builder.py (4)
macro(159-169)macro(553-590)prim_func(152-156)prim_func(659-752)tilelang/language/proxy.py (2)
Ref(269-270)Ref(278-279)
tilelang/language/__init__.py (1)
tilelang/language/proxy.py (2)
Ref(269-270)Ref(278-279)
tilelang/language/v2/builder.py (1)
tilelang/language/proxy.py (2)
Ref(269-270)Ref(278-279)
testing/python/language/test_tilelang_intimm.py (1)
tilelang/language/v2/dtypes.py (4)
int32(156-156)uint32(184-184)int64(157-157)uint64(185-185)
🪛 Ruff (0.14.5)
testing/python/language/test_tilelang_language_frontend_v2.py
425-425: Avoid specifying long messages outside the exception class
(TRY003)
tilelang/language/__init__.py
25-25: 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
🔇 Additional comments (10)
3rdparty/tvm (1)
1-1: Verify submodule commit aligns with PR objectives and contains intended changes.The submodule pointer has been updated from
713e6ade56eaa72cc85d58d9228dd9f34cc2d03etobc31e7ad9f9fafd7659dfabafe359fd55a0ffc1e. However, there's a discrepancy between the PR title ("Remove unused let_bindings_ in CodeGenC to fix #1300") and the AI summary (which describes new Ref type, dtype system changes, and test coverage).Without access to the external TVM repository commit details, I cannot verify:
- What changes this commit introduces
- Whether it addresses the stated goal of fixing #1300
- How it relates to the other changes mentioned in the AI summary
Please confirm that this specific commit contains the intended changes and aligns with the PR's goal. You can verify this by:
- Checking the TVM commit details at
bc31e7ad9f9fafd7659dfabafe359fd55a0ffc1e- Confirming it addresses the let_bindings_ removal mentioned in #1300
- Clarifying whether the AI summary describes changes in the main repository (outside the TVM submodule) or if there's a mismatch in the summaries
tilelang/language/__init__.py (1)
25-25: LGTM! Ref export added correctly.The new
Refsymbol is imported from.proxyand re-exported to support macro argument annotations. The static analysis hint about unusednoqais a false positive—theF401suppression is valid for re-exported symbols in__init__.py.testing/python/language/test_tilelang_intimm.py (1)
6-25: LGTM! Comprehensive integer immediate tests.The test exercises boundary values for signed and unsigned 32/64-bit integers and confirms bitwise operations work correctly. The test validates int32, uint32, int64, and uint64 constructors with edge cases and operator support.
tilelang/language/proxy.py (2)
4-4: LGTM! Import additions for generic Ref type.Generic and TypeVar are added to support the new parameterized Ref type used in macro annotations.
267-279: LGTM! Ref type follows established pattern.The Ref type is defined as Generic[_T] with tir.Var for type checking, and as a placeholder at runtime, mirroring the existing pattern used for Tensor classes (lines 216-265). This dual definition enables proper static type checking while maintaining runtime compatibility.
tilelang/language/v2/builder.py (1)
338-338: LGTM! Clearer warning message.The updated warning now explicitly suggests
T.alloc_var, making it more actionable for developers encountering variable shadowing.tilelang/language/v2/dtypes.py (4)
90-95: LGTM! int_ alias is necessary to avoid shadowing.The int_ alias captures Python's built-in int type before it's shadowed by dtype('int32') at line 314. This allows
isinstance(expr, int_)checks in__dtype_call__to work correctly after the module-levelintreassignment. The implementation correctly returns a constant viatvm.tir.constfor integer literals.
158-161: LGTM! TYPE_CHECKING stubs for new vector dtypes.The new 2-width integer vector type stubs (int8x2, int16x2, int32x2, int64x2, uint8x2, uint16x2, uint32x2, uint64x2) follow the established pattern for existing vector types (x4, x8, x16, etc.) and enable proper static type checking.
Also applies to: 186-189
323-326: LGTM! Runtime dtype definitions for new vector types.The runtime definitions for the 2-width integer vector dtypes are consistent with the TYPE_CHECKING stubs and follow the established pattern used for other vector widths.
Also applies to: 351-354
487-490: LGTM! Public API exposure updated.The _all_dtypes set and public API exposure are correctly updated to include the new 2-width integer vector dtype names, making them accessible throughout the codebase.
Also applies to: 515-518
…le-ai#1305) * [Feat] add missing support of uint32x2 * [Feat] Add `T.Ref` annotation and tests * fix lint error * minor update for error message on twice decl * Remove unused let_bindings_ in CodeGenC to fix tile-ai#1300
This pr remove unused
let_bindings_in tvm CodeGen to fix #1300