-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add split-K control to cuBLAS reduced-precision settings #164766
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/164766
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit cf5223c with merge base 4a6abba ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
3f16d22
to
1f30115
Compare
1f30115
to
52dbd40
Compare
torch/csrc/Module.cpp
Outdated
option == at::CuBLASReductionOption::AllowReducedPrecisionWithSplitK; | ||
bool allow_splitk = option != | ||
at::CuBLASReductionOption::DisallowReducedPrecisionDisallowSplitK; | ||
return Py_BuildValue("(pp)", allow_reduced_precision, allow_splitk); |
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.
maybe it wants something like Py_BuildValue("(OO)", allow_reduced_precision ? Py_True : Py_False, allow_splitk ? Py_True : Py_False);
52dbd40
to
5f0d23d
Compare
5f0d23d
to
cf5223c
Compare
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.
LGTM, though I guess @albanD would be a bit unhappy that it does not follow accelerator generic abstraction
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.
That sounds good to me, this is quite specific for now, so it's ok to have a oneoff API and we can extend it later as needed.
|
||
if isinstance(value, bool): | ||
return value, True | ||
if isinstance(value, (list, tuple)): |
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: the doc says tuple-only
) | ||
|
||
if isinstance(value, bool): | ||
return value, True |
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.
This should query the current split_k value and not force override it to True?
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.
If we want to preserve current behavior we should force-override it to True
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 see, ok
@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 |
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e404623178832f8a3e1d34e1e175da
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela