-
Notifications
You must be signed in to change notification settings - Fork 74
Improve error message for PrecomputedValues
#5700
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
base: main
Are you sure you want to change the base?
Conversation
|
!test |
1 similar comment
|
!test |
|
Review updated until commit f770ae9 Description
|
| Relevant files | |||||
|---|---|---|---|---|---|
| Bug fix |
|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 PR contains tests |
| ⚡ Recommended focus areas for review |
Memory Safety
const Val* ir_node) in the binding_log_. Need to verify that these pointers remain valid throughout the PrecomputedValues object lifetime and that there are no dangling pointer issues when IR nodes are destroyed before the PrecomputedValues object. |
Test failures
-
(Medium, 1)
CUDA out-of-memory in nvFuser TmaPointwiseTestF.SplitGridDim2DTest Name H100 Source TmaPointwiseTestF.SplitGridDim2D ❌ Link
| if (raw_val > 0) { | ||
| for (auto extent : it.second) { | ||
| bindValue(extent->evaluatorIndex(), raw_val); | ||
| bindValue(extent->evaluatorIndex(), raw_val, extent); |
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.
| bindValue(extent->evaluatorIndex(), raw_val, extent); | |
| bindValue(extent->evaluatorIndex(), raw_val, const_cast<Val*>(extent)); |
[Suggested by AI] Change made:
- The third argument passed to bindValue was changed from extent to const_cast<Val*>(extent).
Why:
- bindValue evidently expects a (non-const) Val* for its third parameter.
- extent is (or is treated as) a const Val*. Passing it directly caused a build/ overload-resolution error.
- Using const_cast removes the const-qualification, making the pointer type match the function signature, so the compilation succeeds.
Effect:
- Fixes the build error due to type mismatch.
- Introduces a const-cast, which means the code is now allowed to modify the object referred to by extent through that pointer. This is safe only if extent really points to a mutable object; otherwise it results in undefined behaviour.
| if (raw_val > 0) { | ||
| for (auto extent : it.second) { | ||
| bindValue(extent->evaluatorIndex(), raw_val); | ||
| bindValue(extent->evaluatorIndex(), raw_val, extent); |
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.
| bindValue(extent->evaluatorIndex(), raw_val, extent); | |
| bindValue(extent->evaluatorIndex(), raw_val, const_cast<Val*>(extent)); |
[Suggested by AI] Change made
• In the call to bindValue() the third argument was changed from extent to const_cast<Val*>(extent).
Why
• The variable extent is (or has recently become) a const Val*.
• bindValue() still takes a non-const Val*.
• Passing a const pointer to a function that expects a non-const pointer no longer compiles, so the build failed.
Fix
• const_cast<Val*>(extent) explicitly removes the const-qualification, converting extent from const Val* to Val*, allowing the call to match bindValue()’s parameter list and restoring a successful build.
In short: the patch resolves a type-mismatch build error by casting away constness when passing extent to bindValue().
|
!test |
No description provided.