-
Notifications
You must be signed in to change notification settings - Fork 89
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
Enable split reduce by default #3709
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3709 +/- ##
===========================================
- Coverage 92.23% 92.21% -0.03%
===========================================
Files 514 514
Lines 21746 21750 +4
===========================================
- Hits 20057 20056 -1
- Misses 1689 1694 +5 ☔ View full report in Codecov by Sentry. |
docs/dev/env_vars.rst
Outdated
Enable split_reduce. | ||
.. envvar:: MIGRAPHX_SPLIT_REDUCE_SIZE | ||
Set to the size of the reduction to do a split reduce. | ||
Set -1 to disable split reduce. |
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.
Would it make sense to have an example of a size? Are you talking bytes? Suggest adding an example and what the default is now
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.
Would it make sense to have an example of a size?
What do you mean example?
Are you talking bytes?
The number of elements, we dont ever measure the size of a reduction in terms of bytes.
Suggest adding an example
Like MIGRAPHX_SPLIT_REDUCE_SIZE=8192
?
what the default is now
The default can change depending on the backend.
@@ -35,6 +35,7 @@ struct module_pass_manager; | |||
|
|||
struct MIGRAPHX_EXPORT fuse_pointwise_reduce | |||
{ | |||
std::size_t split_size = 8192; |
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.
Is this the default split of 8192?
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.
Thats the value when using the default constructor.
This build is OK for merge ✅ |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
This will also enable pointwise fusions around broadcasts by default. An environment variable is added to adjust the size if we want to experiment with different sizes.