-
Notifications
You must be signed in to change notification settings - Fork 333
Modify the SM architecture number to support Thor’s sm110. #957
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! 🚀 |
WalkthroughUpdated TargetIsSm100 in src/target/utils.cc to expand the accepted CUDA SM architecture range from 100–103 to 100–110. The function still checks CUDA targeting first and retrieves the architecture via GetArchInt; other control flow and checks remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
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 |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/target/utils.cc(1 hunks)
🔇 Additional comments (1)
src/target/utils.cc (1)
60-60: Verify SM111–SM119 support and bound inclusivity
- RG search shows no references to SM111–SM119; confirm whether architectures 111–119 exist or are planned.
- Align upper-bound logic with other ranges (e.g., use
< 111instead of<= 110if exclusive).
| return false; | ||
| int arch = GetArchInt(target); | ||
| return arch >= 100 & arch <= 103; | ||
| return arch >= 100 & arch <= 110; |
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.
Critical: Use logical AND (&&) instead of bitwise AND (&).
Line 60 incorrectly uses the bitwise AND operator & instead of the logical AND operator &&. While this may accidentally produce correct results due to implicit bool-to-int conversion, it is incorrect and inconsistent with all other architecture check functions in this file (lines 32, 39, 46, 53, 67).
Apply this diff to fix the operator:
- return arch >= 100 & arch <= 110;
+ return arch >= 100 && arch <= 110;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return arch >= 100 & arch <= 110; | |
| return arch >= 100 && arch <= 110; |
🤖 Prompt for AI Agents
In src/target/utils.cc around line 60, the range check uses a bitwise AND (`&`)
instead of a logical AND; replace the operator so the return becomes a proper
boolean range test (use `&&` between the two comparisons, e.g. return arch >=
100 && arch <= 110;) to match the other architecture check functions and ensure
correct logical behavior.
Modified the architecture number in sm100 to support Thor’s execution. The results are as follows.
Summary by CodeRabbit