-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
MKLDNN layout: Support for batch norm operator #11098
MKLDNN layout: Support for batch norm operator #11098
Conversation
69b8f56
to
7d56435
Compare
@luotao1 The code is prepared to the code-review process. |
bool is_diff_dst_reordered = false; | ||
if (diff_dst_pd != user_diff_dst_memory.get_primitive_desc()) { | ||
diff_dst_memory = memory(diff_dst_pd); | ||
reorder_diff_dst = reorder(user_diff_dst_memory, diff_dst_memory); |
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.
Inplace reorder?
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.
@tensor-tang Yes.
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.
But does mkldnn reorder support inplace?
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.
"Both forward and backward passes support in-place operation, i.e. src and dst point to the same memory for forward, and diff_dst and diff_src point to the same memory for backward pass."
info
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.
Actually my point is does mkldnn::reorder
support inplace, not batchnorm
.
AFAIK, mkldnn::reorder
does not support inplace yet.
While, you are not using inplace reorder here.
auto diff_dst_memory = user_diff_dst_memory;
...
diff_dst_memory = memory(diff_dst_pd);
This will allocate new memory.
So that's fine.
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.
All unit tests passed.
LGTM
Waiting for supports of the mkldnn’s layout.
Please have a look at this batch norm operator supported by the MKLDNN’s layout and assess if we can keep this design of the code.
This code uses the implementation of layout which is available in the last pull-request. Therefore some of the function in this code are not available in this pull-request, but are available here.
This version of code can be merged into the main branch when the pull-request with layout is accepted.
The concept of splits of the long code was proposed by @luotao1
Pull-request is related to #11040