[Language] Fix gemm syntax highlight#1476
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! 🚀 |
WalkthroughReplaced the module-level Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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)
tilelang/language/gemm_op.py (1)
187-188: Consider removing commented-out code.The commented-out static alias is no longer needed since the new dynamic dispatch function replaces it. Keeping dead code can clutter the codebase.
🔎 Proposed cleanup
-# gemm = gemm_v1 if _env.use_gemm_v1() else gemm_v2 -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tilelang/language/gemm_op.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tilelang/language/gemm_op.py (1)
tilelang/env.py (1)
use_gemm_v1(284-290)
⏰ 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). (2)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.1 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
SiriusNEO
force-pushed
the
chaofan/fix3
branch
from
b0b0804 to
bbe0e61
Compare
tilelang/language/gemm_op.py
Outdated
| # gemm = gemm_v1 if _env.use_gemm_v1() else gemm_v2 | ||
|
|
||
|
|
||
| def gemm(*args, **kwargs): |
There was a problem hiding this comment.
one small question, if we replace the alias with function proxy, can lint still tell us the arguments of gemm is?
There was a problem hiding this comment.
Maybe we should explicit add arguments in the sigature
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tilelang/language/loop.py (2)
181-184: Consider adding return type hint and docstring for public API consistency.The
Serialwrapper correctly forwards all parameters toserial. However, as a public API function, it would benefit from:
- A return type hint (
-> frame.ForFrame) matching the underlyingserialfunction- A docstring explaining its purpose and relationship to
serial🔎 Suggested improvements
def Serial( start: tir.PrimExpr, stop: tir.PrimExpr | None = None, step: tir.PrimExpr | None = None, *, annotations: dict[str, Any] | None = None -): +) -> frame.ForFrame: + """Serial for loop (tile-level wrapper). + + Uppercase alias of serial() to emphasize tile-level loop semantics. + See serial() for parameter details. + """ return serial(start, stop, step, annotations=annotations)
187-196: Consider adding return type hint and docstring for public API consistency.The
Unrollwrapper correctly forwards all parameters tounroll, including theexplicitandunroll_factorkeyword arguments. However, as a public API function, it would benefit from:
- A return type hint (
-> frame.ForFrame) matching the underlyingunrollfunction- A docstring explaining its purpose and relationship to
unroll(the underlying function already has comprehensive documentation at lines 122-148)🔎 Suggested improvements
def Unroll( start: tir.PrimExpr, stop: tir.PrimExpr | None = None, step: tir.PrimExpr | None = None, *, explicit: bool = False, unroll_factor: int | None = None, annotations: dict[str, Any] | None = None, -): +) -> frame.ForFrame: + """Unrolled for loop (tile-level wrapper). + + Uppercase alias of unroll() to emphasize tile-level loop semantics. + See unroll() for parameter details. + """ return unroll(start, stop, step, explicit=explicit, unroll_factor=unroll_factor, annotations=annotations)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tilelang/language/gemm_op.pytilelang/language/loop.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tilelang/language/gemm_op.py
🧰 Additional context used
🧬 Code graph analysis (1)
tilelang/language/loop.py (1)
tilelang/language/tir/ir.py (2)
serial(9-28)unroll(75-101)
⏰ 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). (3)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.1 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tilelang/language/gemm_op.py (2)
188-189: Consider removing the commented-out alias.The commented-out code serves as a reference for the old implementation pattern, but once the new function is stable, this comment could be removed to keep the codebase clean.
191-222: Excellent refactoring that addresses all previous concerns.The implementation correctly:
- Calls
_env.use_gemm_v1()without arguments (fixing the previous critical bug)- Uses the implementation-selection pattern suggested in past reviews
- Provides an explicit signature that enables proper IDE support and syntax highlighting (the PR objective)
- Forwards all parameters correctly to the selected implementation
The dispatch now happens at call time rather than module load time, which is appropriate here since it enables better tooling support and allows runtime configuration changes if needed.
Minor: Fix docstring typo.
Line 214: "Int identifier" should be "Integer identifier" for consistency with standard documentation style.
🔎 Suggested fix
- wg_wait (int): Int identifier of the warpgroup MMA batch to wait on.. Defaults to 0. + wg_wait (int): Integer identifier of the warpgroup MMA batch to wait on. Defaults to 0.Note: Also fixes the double period ("..") typo.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tilelang/language/gemm_op.pytilelang/language/loop.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tilelang/language/loop.py
🧰 Additional context used
🧬 Code graph analysis (1)
tilelang/language/gemm_op.py (3)
tilelang/language/v2/annot.py (1)
Buffer(562-585)tilelang/tileop/gemm/gemm_base.py (1)
mbar(128-129)tilelang/env.py (1)
use_gemm_v1(284-290)
⏰ 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: Test for Python 3.12 with Metal (on macos-latest)
2 participants
Following #1466, this PR fixes the highlight problem of
T.gemmSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.