MKLDNN sum OP: implement primitive cache and class refact #12
Conversation
This commit may add some overhead of managing NDArray for each fallback.
Conflicts: src/operator/nn/mkldnn/mkldnn_batch_norm-inl.h
2. Add memory into signature; 3. Try to split BatchNorm into .h file and .cc file. Will finish it after backward code is refactored.
Caching primitive for BatchNorm forward computation
| return this->num_args == other.num_args; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
why do you create a new parameter? You don't have to use MKLDNNParamOpSign. You can use MKLDNNOpSignature.
There was a problem hiding this comment.
The parameter definition is moved from src/operator/tensor/elemwise_sum.cc to this header file in order to be used by mkldnn_sum.cc, in order to support "add_n"(alias "ElementWiseSum") OP which has valid param, I used MKLDNNParamOpSign.
| param = nnvm::get<ElementWiseSumParam>(attrs.parsed); | ||
| } else { | ||
| memset(¶m, 0, sizeof(param)); | ||
| } |
There was a problem hiding this comment.
In case MKLDNNSumCompute() is invoked via "add_n"(alias "ElementWiseSum") OP, the param will exists, so in order to adapt for these 2 scenarios and provide a unified interface, set param to 0 in case there is no param.
src/operator/nn/mkldnn/mkldnn_sum.cc
Outdated
| private: | ||
| std::shared_ptr<mkldnn::sum> fwd; | ||
| std::vector<std::shared_ptr<mkldnn::memory>> in_data; | ||
| mkldnn_output_t out; |
There was a problem hiding this comment.
you shouldn't use mkldnn_output_t. this is designed to use with CreateMKLDNNMem and CommitOutput. it's not supposed to use for holding the mkldnn memory because the second field is a raw pointer. We shouldn't use a raw pointer to hold memory.
There was a problem hiding this comment.
Thanks for the suggestion, will change to use std::shared_ptr to hold the output memory.
src/operator/nn/mkldnn/mkldnn_sum.cc
Outdated
| CommitOutput(out_data, out_mem); | ||
| stream->RegisterPrim(*(this->fwd)); | ||
| auto out_mem = CreateMKLDNNMem(output, this->fwd_pd->dst_primitive_desc(), | ||
| req); |
There was a problem hiding this comment.
you shouldn't call CreateMKLDNNMem twice. The way you restructure the code makes it difficult to work with the interface I designed.
There was a problem hiding this comment.
In order to only invoke CreateMKLDNNMem() once and keep a unified SetDataHandle/Execute API interface (pass NDArray as parameters), one way is to define 2 members under
MKLDNNSumFwd: "std::shared_ptrmkldnn::memory out" and "OutDataOp op" to save created memory and data_op and use it later (maybe a little ugly). Please let me know your suggestions as well if there is better choice.
zheng-da
force-pushed
the
refactor
branch
8 times, most recently
from
c5b06e8
to
8ba9736
Compare
* Added tutorial for FIT API * Added tests for Fit API tutorial * Updated index.md for the new tutorial to show up * Addressed PR feedback * Addressed PR feedback * Removed spurious comment for Py2 and Py3 compatibility * Address PR feedback * Addressed PR feedback * Fixed typo * Added example to showcase custom event handler * Fixed imports as estimator moved to contrib package * Added a side note to inform about estimator reference being updated by the handlers * Corrected typo * update tutorial * address comments * new line * fix import * fix cached graph * fix import * address comments * fix doc gen * add softmax * add to website index * fix doc string * Fix doc gen (zheng-da#12) * fix warining * fix test * fix * fix * fix print * fix test (zheng-da#13) * fix warning (zheng-da#14) * fix href (zheng-da#15)
Description
Checklist
Essentials
make lint)Comments