-
Notifications
You must be signed in to change notification settings - Fork 6.8k
mkldnn s8 conv API change for master #13903
Conversation
@mxnet-label-bot add [pr-awaiting-review, MKLDNN] |
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
Aligned with #13905 which has been merged into 1.4.x
@@ -168,16 +168,28 @@ nnvm::Symbol NDArray::get_autograd_symbol() const { | |||
|
|||
#if MXNET_USE_MKLDNN == 1 | |||
|
|||
NDArray::NDArray(const mkldnn::memory *mkldnn_mem, bool static_data) |
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.
it seems the old impl is incorrect. what if static_data=false
?
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 static_data=false, then NDArray will take responsible to free this memory at deconstruction. But it has a double free risk that if mkldnn_mem be freed outside. I agreed that it's not a good design, so I changed it to std::shared_ptrmkldnn::memory to avoid the chance of double free.
LGTM |
Description
This is extracted from #13697, to make API change for master.
@KellenSunderland @pengzhao-intel
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments