Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Softmax for find20 #2776
Softmax for find20 #2776
Changes from 29 commits
b985479
2611905
07295fd
4ffa3ca
343db12
08fe02e
a500ffa
60eb817
22d0557
b5c5f39
4b348c2
3d15064
e278107
800d0bd
f96c834
9b05596
9fe01b0
e0d5e57
8a566a5
96bb3ee
d5e5fa1
2169f9e
e8b2edc
d407f53
2694e4b
b3db167
a9984e2
33b3f0a
3c7fb96
88dd97c
3754f93
9384e0f
ad2d52e
18cb331
5149aa0
7990828
dba6f95
43abd49
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would rather store them as
bool
. This way later it would be harder to accidentally use them instead of actual values. On subsequent runs float values here would be incorrect anyway.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.
[Performance] @Vsevolod1983 See #2671 (review)
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 removed alpha and beta from network config in softmax primitive. Is it enough for the this PR ?
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.
@Vsevolod1983 I would begin with removing alpha/beta from PD (and then fix build errors by forwarding alpha/beta via InvokeParams). However I think we need to discuss #2671 (review) first.
/cc @CAHEK7 @DrizztDoUrden
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.
Should we do this kind of refactoring as a separate ticket / PR ?
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.
We need. Actually a/b affects kernel compilation, like it does for convolutions. There are some optimizations with default a/b values, so we need at least a "default a/b" flag in the problem description and in the network config.
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 restored alpha and beta in network config in this PR until we decide what exactly we want to do next.
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.
[Informative] @Vsevolod1983 @CAHEK7 it's interesting that "default alpha" optimization won't give us much, but "default beta" opt removes 1 global memory read, which may be substantial (./src/kernels/MIOpenSoftmax.cl)
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.
@atamazov right now
default alpha
gives much since it enables attention softmax solver, which is faster and does not support a/b in terms of normal softmax operation.Both values are used in the
isApplicable
method and can't be just removed.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.
[Can be postponed] I think we can statically initialize
solvers
.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.
@DrizztDoUrden Can you please suggest @Vsevolod1983 the technique for doing this? Thanks.
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.
The most basic primitive for this is
MIOpen/src/include/miopen/find_solution.hpp
Line 181 in 9db0c26
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 is postponable in contrast to a/b problem