-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[Inductor] Update DeviceAssert op to behave like store #163696
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
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163696
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 21310b2 with merge base df4ebdd ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torch/_inductor/codegen/common.py
Outdated
| raise NotImplementedError | ||
|
|
||
| def device_assert_async(self, cond: CSEVariable, msg: str) -> None: | ||
| raise NotImplementedError |
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.
nit: make this have a message like the other one?
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.
thanks, I've added the similar message.
torch/_inductor/ops_handler.py
Outdated
| return val | ||
|
|
||
| def device_assert_async(self, *args, **kwargs) -> None: | ||
| raise NotImplementedError("device_assert_async not implemented") |
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.
similar comment, message consistency?
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.
I've updated it.
Updated the DeviceAssert operation to match the behavior of Store, it will fixes the issue mentioned in [this PR](#163023) and updated testcases as Elias [suggested](#160677 (comment)). cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben mlazos choijon5 [ghstack-poisoned]
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Updated the DeviceAssert operation to match the behavior of Store, it will fixes the issue mentioned in [this PR](#163023) and updated testcases as Elias [suggested](#160677 (comment)). Pull Request resolved: #163696 Approved by: https://github.com/mlazos
Stack from ghstack (oldest at bottom):
Updated the DeviceAssert operation to match the behavior of Store, it will fixes the issue mentioned in this PR and updated testcases as Elias suggested.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben @mlazos @choijon5