[oneDNN] Disable caching of Reorder operation#35664
[oneDNN] Disable caching of Reorder operation#35664lidanqing-vv merged 11 commits intoPaddlePaddle:developfrom
Conversation
|
Thanks for your contribution! |
|
@jczaja Hi please update this PR because there is file conflict. |
jczaja
force-pushed
the
prv-disable-cache-reorder
branch
from
5d71444 to
13439fb
Compare
| auto slice_mem_p = reorder_handler.AcquireSubmemory(slice_dims, offsets, | ||
| reorder_src_memory_p); | ||
| auto reorder_dst_memory_p = reorder_handler.AcquireDstMemory( | ||
| out, slice_dims, 0, get_plain_format_tag(x), ctx.GetPlace()); |
There was a problem hiding this comment.
My bad when resolving conflicts. Good catch! Thanks
jakpiase
left a comment
jakpiase
left a comment
There was a problem hiding this comment.
LGTM, I hope that oneDNN's caching can deal with submemories
| const MKLDNNMemoryFormat& fmt, void* ptr) { | ||
| return this->AcquireMemory(dims_, dtype_, fmt, ptr, "@user_src_mem_p"); | ||
| auto md = mkldnn::memory::desc(dims_, dtype_, fmt); | ||
| return std::make_shared<mkldnn::memory>(md, engine_, ptr); |
There was a problem hiding this comment.
If we're no longer caching those dnnl::memory objects then why returning them wrapped in shared_ptr? Why not just returning the plain object?
| } | ||
| return reorder_p; | ||
| const MKLDNNMemoryFormat& fmt, platform::Place place) { | ||
| auto dst_md = platform::MKLDNNMemDesc(dims, dtype_dst_, fmt); |
There was a problem hiding this comment.
In the above overload of this function you use this class member dims_, but here you use the parameter dims Why this discrepancy? Why can you create the ReorderMKLDNNHandler class object with a dims passed in constructor and then use it with different dims? Why then store output dims as a member?
| }; | ||
|
|
||
| class ReorderMKLDNNHandler : public MKLDNNHandler { | ||
| class ReorderMKLDNNHandler { |
There was a problem hiding this comment.
I would encourage you to extract this class to separate file. It's not a template so it would make a binary size smaller and compilation faster.
| framework::proto::VarType::Type vtype_, vtype_dst_; | ||
| mkldnn::memory::data_type dtype_, dtype_dst_; |
There was a problem hiding this comment.
What is the benefit of storing both vtype_ and dtype_? Why not just vtype_ since we can easily convert VarType::Type to oneDNN type?
* - REorder disabling caching * - compilation fix * - another compilation fix * - another compilation fix * - compilation fix * - Fix * - yet another compilation fix * - suppresingly another compilation fix * - lint * - fix after review * - fix
4 participants
PR types
Others
PR changes
Others
Describe
This is another one in a series of disabling PAddlePaddle caching of oneDNN objects, this time for reorder operation